Update ColorCoutSink for WIN32 #132
Conversation
| #ifdef _WIN32 | ||
|
|
||
| #ifndef WIN32_LEAN_AND_MEAN | ||
| #define WIN32_LEAN_AND_MEAN 1 |
There was a problem hiding this comment.
this doesn't seem to be used. please remove or explain the purpose
There was a problem hiding this comment.
That is to avoid extra inclusions to speed up the compilation, see https://learn.microsoft.com/en-us/windows/win32/winprog/using-the-windows-headers
| LOG(FATAL) << "This call will exit the example. A failed CHECK call, \n" | ||
| << "a LOG(FATAL) or a fatal crash (seg fault or similar) will show up in RED\n" | ||
| << "\n\nThis call will also be DUPLICATED. One FATAL LOG message will be shown through the \n" | ||
| << "the crash handler and sent to cout, the LOG call will asynchronously send the LOG output to cout.\n" |
There was a problem hiding this comment.
Only formatting difference. It's only a snippet and the change doesn't have a big impact so I'm fine with the formatting changes
There was a problem hiding this comment.
Oh ops the formatting wasn't supposed to be pushed, the code is that it has a missing ";" so that's why the line was modified. I can keep it that way if you want.
There was a problem hiding this comment.
thanks for the observation and fix
| struct ColorCoutSink { | ||
| #ifdef _WIN32 |
There was a problem hiding this comment.
Since it's a snippet I think this is fine. For more production-type code I'd suggest avoiding #ifdef separated like the plague. A much cleaner approach is to do something like
#ifdef _WIN32
#include <ColoredCoutSinkWindowsImpl.h>
#else
.... this file ... or include <ColoredCoutSinkNixImpl.h>
You can consider this but in case you don't want to spend time on cleaning it up, I think that is fine
There was a problem hiding this comment.
I.e. the approach would potentially add two .h files more but each file and both Nix and Wind code would stay clean.
There was a problem hiding this comment.
I could make two different header files if you want to, originally it was supposed to have the ifdefs because I also added the filter code inside the ColorCoutSink (much like LogRotateWithFilter), hence why it was easier for me to have the ifdef.
There was a problem hiding this comment.
sure, if you don't mind. Either two header files or one file but keeping all the content separate.
it's a headache to have to read #ifdef's inside functions
| struct ColorCoutSink { | ||
| #ifdef _WIN32 | ||
| ColorCoutSink() : hHandle_(GetStdHandle(STD_OUTPUT_HANDLE)) {} |
There was a problem hiding this comment.
it's annoying that the WriteConsole output is completely hidden in the console
Maybe an artifact of the appveyor?
Do you know if it's possible to see this displayed to cout?
https://ci.appveyor.com/project/KjellKod/g3sinks/builds/45527002
|
I've now divided the two sinks into two different files as discussed in the comments. |
The following PR adds support of the Color sink for WIN32 consoles (by using WIN32 api), I have also enabled the test for Windows.
A preview of how it looks like can be seen here: https://i.imgur.com/YJ3BC6n.png
Notes: