Feature/cleanup cmake deps #61
Conversation
|
|
||
| } // namespace | ||
|
|
||
| AslFormatter::AslFormatter() = default; |
There was a problem hiding this comment.
Is there a reason for defaulting constructors in the src file? I'm more used to seeing that in the header (and vice versa, if I see a constructor declaration I assume it's non-default)
There was a problem hiding this comment.
I think I was just trying to move all definitions to the source file for compilation speed reasons, but yeah not super necessary / makes more sense in the header file
| } // namespace | ||
|
|
||
| // TODO(nathan) add warning | ||
| GlogLogger::GlogLogger() { |
There was a problem hiding this comment.
Does it make sense to make the glog logger available and then default to stdout or would it make more sense to just not register it if glog is not included? Slight preference for not registering it, although this gives more explicit warnings about glog not being compiled.
config_utilities/src/log_to_glog.cpp
Outdated
|
|
||
| } // namespace | ||
|
|
||
| // TODO(nathan) add warning |
There was a problem hiding this comment.
Are these two todos still relevant?
@Schmluk some cleanup I was doing a little while ago that I never got around to PRing (I was trying to fix some stuff with how we find glog/gflags downstream / think I was using
semantic_inferencewithout glog installed and having issues). I need to double check that this still builds cleanly with everything downstream before this is actually good to go, but I did check that the conditional glog implementation works with/without glog installed