Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Elapsed time larger than 6 digits ignores alignment width #1327

Closed
ivan236634452 opened this issue Nov 26, 2019 · 3 comments
Closed

Elapsed time larger than 6 digits ignores alignment width #1327

ivan236634452 opened this issue Nov 26, 2019 · 3 comments

Comments

@ivan236634452
Copy link

ivan236634452 commented Nov 26, 2019

Tested in release 1.4.2 and today's v1.x branch. All alignments are affected.

Code to reproduce (right alignment):

spdlog::set_pattern("|%12u|");
spdlog::info("");
std::this_thread::sleep_for(std::chrono::milliseconds(1500));
spdlog::info("");
spdlog::info("");

spdlog::set_pattern("|%12i|");
spdlog::info("");
std::this_thread::sleep_for(std::chrono::milliseconds(1500));
spdlog::info("");
spdlog::info("");

Expected output:

|      001100|
|  1514900200|
|      173200|
|      000004|
|     1515305|
|      000196|

Actual output:

|      001100|
|      1514900200|
|      173200|
|      000004|
|      1515305|
|      000196|
@gabime
Copy link
Owner

gabime commented Nov 28, 2019

Thanks for reporting.

@ivan236634452
Copy link
Author

ivan236634452 commented Nov 29, 2019

The culprit is "format" method in elapsed_formatter class in "pattern_formatter-inl.h":

void format(const details::log_msg &msg, const std::tm &, memory_buf_t &dest) override
{
    auto delta = (std::max)(msg.time - last_message_time_, log_clock::duration::zero());
    auto delta_units = std::chrono::duration_cast<DurationUnits>(delta);
    last_message_time_ = msg.time;
    ScopedPadder p(6, padinfo_, dest);
    fmt_helper::pad6(static_cast<size_t>(delta_units.count()), dest);
}

I would've actually preferred to get rid of leading zeroes altogether. For me this:

|         2 |
|     14189 |
|        30 |
|         0 |
|       178 |
|        10 |
|         1 |
|    345212 |

reads easier than this:

|    000002 |
|    014189 |
|    000030 |
|    000000 |
|    000178 |
|    000010 |
|    000001 |
|    345212 |

My current fix is this:

void format(const details::log_msg &msg, const std::tm &, memory_buf_t &dest) override
{
    auto delta = (std::max)(msg.time - last_message_time_, log_clock::duration::zero());
    auto delta_units = std::chrono::duration_cast<DurationUnits>(delta).count();
    last_message_time_ = msg.time;
    const auto field_size = fmt_helper::count_digits(delta_units);
    ScopedPadder p(field_size, padinfo_, dest);
    fmt_helper::append_int(delta_units, dest);
}

But it is slower (probably), and breaks backward compatibility (to a significantly greater extent). But I'm just a scrub user, not a library designer, and can afford to not care about the latter.

@gabime gabime closed this as completed in 2d4e531 Nov 29, 2019
@gabime
Copy link
Owner

gabime commented Nov 29, 2019

@ivan236634452 Fixed. I agree it is more readable, so I changed according to your proposal. It is also few nanos faster!

bachittle pushed a commit to bachittle/spdlog that referenced this issue Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants