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

UTC Time #451

Merged
merged 4 commits into from
May 31, 2017
Merged

UTC Time #451

merged 4 commits into from
May 31, 2017

Conversation

ThePhD
Copy link
Contributor

@ThePhD ThePhD commented May 30, 2017

This issue addresses #215 in the way @gabime stated he'd like it to be handled: with a simple flag that can be passed to pattern_formatter's constructor. It's a smaller, simpler solution that propagates its changes to the virtual functions _set_pattern and set_pattern in spdlog::logger and spdlog::async_logger.

The implementation is made as an enumeration simply because there may be more efficient time abstractions that spdlog takes advantage of in the future: the enumeration allows for seamless transition to these while maintaining backwards compatibility by ensuring that all defaulted loggers and pattern formatters use spdlog::pattern_time::local by default.

Anyone who has derived from logger and async_logger will have to change their code to handle the new parameter for the virtual set_pattern call, but I believe this is a negligible burden in 100% of the cases where people have actually customized spdlog to this degree.

I have not measured the impact of a switch statement, but have done my best to use my general knowledge to ensure it will remain more or less swift, especially in the default case.

Small question: did you mean to never initialize the member variable _pattern in spdlog::pattern_formatter ? It never gets initialized (and you never use it anywhere?). I would imagine it's accidentally an artifact of slowly changing the pattern formatter to work in a compiled form and actually no longer needing it, so I don't set it in my code, but I do set the _time member variable. Shouldn't it be removed, if that's the case? If so, I will be happy to remove it in this branch.

Let me know if you need anything else.

ThePhD added 2 commits May 30, 2017 18:05
…ver the type; we make it an enum for possible expansions of time abstractions that might make it into the C++ standard in the future (see Howard Hinnant's date/timezone library) or might be usefully-available from the OS at some point in time
…gabime, or a reflection of a refactor with some data member left behind?
Copy link
Owner

@gabime gabime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. R_formatter should do HH:MM only

  2. I am not sure what's the point in the lambda. Why not simple

if (ptime == pattern_time::local) 
  return details::os::localtime(log_clock::to_time_t(msg.time));
else 
  return details::os::gmtime(log_clock::to_time_t(msg.time));
  1. Please change the pattern_time enum name to pattern_time_type

  2. Please change the ptime variable name to time_type

…ications outside of standard are probably unlikely anyhow)

pattern_time -> pattern_time_type
ptime variable name -> pattern_time variable name
make sure four spaces used, not tabs
@ThePhD
Copy link
Contributor Author

ThePhD commented May 31, 2017

  1. (Fixed) Oops, that was a mistake from something else.

  2. (Fixed) I'll change it to the if statement. I use a lambda so I could early-return and I only used it once, but I can just make it a private member function instead.
    2a. (Rationale Explanation, Not Actually Done) If I remove the "default" statement and use a switch statement, then gcc/clang will warn if the switch statement doesn't use all the enum values of pattern_time_type. This might be useful for catching unupdated code in the future if more pattern_time_type values are added. But, I'm still leaving it as the if-statement as you requested!

3 & 4. (Fixed) I was wondering what would be a good way to name them. Changing to pattern_time_type frees up the variable name pattern_time: done!

@gabime gabime merged commit 5d5f2f3 into gabime:master May 31, 2017
@ThePhD ThePhD deleted the pattern_time branch May 31, 2017 18:40
bachittle pushed a commit to bachittle/spdlog that referenced this pull request 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

Successfully merging this pull request may close these issues.

2 participants