Logging improvements: options & custom log record handling #167
Conversation
|
The files you have added are lacking the license info and copyright notice. Furthermore, there are no unit tests for the stand-alone logger. You only modified an existing test but you should create a new one to test new code and to revert the original tests. |
|
Ok I'll remove the test, add a proprer test and fix indentation and copyright. |
|
Yes, please put your name in the copyright. |
| class log_record | ||
| { | ||
| public: | ||
| log_record(){} // = default; |
There was a problem hiding this comment.
Just use =default now that we're C++11 only anyway.
There was a problem hiding this comment.
I agree, I just cannot compile this at this time as I'm using VS2012 and don't have easy access to other compiler for some time. I'll do it anyway and try to compile it on my server.
|
Just an additional comment: I will do the improvements in the coming days, not today (I'm traveling for business). I think I'll be able to compile on another compiler before next week. |
Some temporary files got into the last commit, removed them now.
- log record informations are now separated - the log handler decide how to format the log record - log record accepts more than compatible-to-string types on construction, anything string-streamable will work (need additional tests though)
| @@ -11,16 +12,26 @@ | |||
| print out network-related errors through standard error. This is only | |||
| useful when NETWORK_DEBUG is turned on. Otherwise the macro amounts to a | |||
| no-op. | |||
| The user can force the logging to be enabled by defining NETWORK_ENABLE_LOG. | |||
There was a problem hiding this comment.
Nit: Can you move this down one line? 😀
|
LGTM Can you pull from master, merge locally (on your machine), then publish again to your master? I'll want to be able to merge this automatically. |
Conflicts: libs/network/src/CMakeLists.txt libs/network/test/CMakeLists.txt
| } | ||
|
|
||
| std::string message() const { return m_text_stream.str(); } | ||
| const std::string& filename() const { return m_filename; } |
There was a problem hiding this comment.
This doesn't need to return a const ref, it can better return a value IMO.
There was a problem hiding this comment.
Sure? Even if it's only to read the value? I know move semantics makes things cheaper but they keep saying it's still not free. I'll do it if you confirm you prefer by value, I have no problem with that. :)
There was a problem hiding this comment.
I don't think returning a value is necessary here. It's harmless to return a reference at this point.
Logging improvements: options & custom log record handling
|
Well done Joël, thanks for this PR! |
This PR adds
Initial discussion started here: https://groups.google.com/d/msg/cpp-netlib/KeR_uh7wl2A/tirSOCJsf_8J
Current discussion is going on there: https://groups.google.com/forum/?fromgroups=#!topic/cpp-netlib/Uv98-L44OwE
This is subject to discussion and improvements.
Note: the log_record object will certainly evolve in the future.