Auto-detect whether Benchmark should produce colorized output #126
Conversation
| #endif | ||
|
|
||
| namespace benchmark { | ||
| namespace posix { |
There was a problem hiding this comment.
I know the top level namespace posix is reserved for the implementation in C++. I would prefer not using a nested one either. Could you come up with another name?
There was a problem hiding this comment.
My model here was Google Test's internal/gtest-port.h. I'll have a ponder about an alternate namespace name. Would you be open to ditching the nested namespace altogether?
There was a problem hiding this comment.
Not opposed towards dropping the namespace.
|
Seems like a reasonable patch. Is there a reason to perfer using C strings in the function interfaces as opposed to std::string? If there isn't then I would prefer to use std::string. |
| @@ -858,7 +862,7 @@ void PrintUsageAndExit() { | |||
| " [--benchmark_min_time=<min_time>]\n" | |||
| " [--benchmark_repetitions=<num_repetitions>]\n" | |||
| " [--benchmark_format=<tabular|json|csv>]\n" | |||
| " [--color_print={true|false}]\n" | |||
| " [--color_print={auto|true|false}]\n" | |||
There was a problem hiding this comment.
the usage says 'true' and 'false' while the help says 'yes' and 'no'.
| ParseInt32Flag(argv[i], "v", &FLAGS_v)) { | ||
| for (int j = i; j != *argc; ++j) argv[j] = argv[j + 1]; | ||
|
|
||
| --(*argc); | ||
| --i; | ||
| } else if (IsFlag(argv[i], "help")) { | ||
| } else if (IsFlag(argv[i], "help") || HasBenchmarkFlagPrefix(argv[i])) { |
There was a problem hiding this comment.
!HasBenchmarkFlagPrefix?
also, redundant given how we parse the strings above.
There was a problem hiding this comment.
Not quite -- without this check, you could pass --benchmark_format or similar with no associated value, and no error will be reported.
There was a problem hiding this comment.
$ test/benchmark_test --benchmark_format=foo
benchmark [--benchmark_list_tests={true|false}]
[--benchmark_filter=]
[--benchmark_min_time=<min_time>]
[--benchmark_repetitions=<num_repetitions>]
[--benchmark_format=<tabular|json|csv>]
[--color_print={true|false}]
[--v=]
this is working as intended. the checks for specific args come next.
|
@nickhutchinson are you still working on this? |
|
@nickhutchinson Are you interested in continuing with this? |
|
Sorry for the delay, I’ll try fix this up this week.
|
|
Finally got around to rebasing this on master -- thanks for your patience. I believe I've addressed the review comments, in that the new patch:
I've tested on Win10/MSVC2015 and OS X 10.11/Clang 3.8. |
|
❌ Build benchmark 445 failed (commit a7ed56c34b by @nickhutchinson) |
| return (env != nullptr && env[0] != '\0') ? env : nullptr; | ||
| #else | ||
| return getenv(name); | ||
| #endif // BENCHMARK_OS_WINDOWS |
|
Thanks again for this patch! PS. I really liked your command line argument checking. If you want to submit that as a separate patch it would be very welcome. |
Rename --color_print to --benchmark_color for consistency with the other flags (and Google Test). Old flag name is kept around for compatibility. The --benchmark_color/--color_print flag takes a third option, "auto", which is the new default. In this mode, we attempt to auto-detect whether to produce colorized output. (The logic for deciding whether to use colorized output was lifted from GTest: <https://github.com/google/googletest/blob/master/googletest/src/gtest.cc#L2925>.)
|
Addressed code review comments.
|
|
✅ Build benchmark 452 completed (commit b6cc5dede9 by @nickhutchinson) |
Attached is a patch that makes Benchmark only produce colorized output
if stdout is a TTY. (This can be overridden with --color-print=true.)
The logic for deciding whether to use colorized output was lifted from
GTest: https://code.google.com/p/googletest/source/browse/trunk/src/gtest.cc#2872.
I haven't got a Windows system to test my changes on, but I see from #7
that there's more work to be done here anyway, so hopefully that's okay!