Clang tidy fixes for Common++ #1884
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #1884 +/- ##
========================================
Coverage 82.84% 82.85%
========================================
Files 291 291
Lines 51565 51567 +2
Branches 11386 11433 +47
========================================
+ Hits 42721 42724 +3
+ Misses 8021 7681 -340
- Partials 823 1162 +339
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Common++/src/Logger.cpp
Outdated
| std::cerr << std::left << "[" << std::setw(5) << Logger::logLevelAsString(logLevel) << ": " << std::setw(45) | ||
| << sstream.str() << "] " << logMessage << std::endl; | ||
| << sstream.str() << "] " << logMessage << '\n'; |
There was a problem hiding this comment.
We always use std::endl, not sure why \n is better, but maybe we should stay consistent and always use std::endl
There was a problem hiding this comment.
Because '\n' does not flush the stream buffer. std::endl flushes it every time it is encountered.
For std::cerr that doesn't do much as that stream is unbuffered, but it slows down std::cout a lot if many flushes are done unnecessarily, so it is generally more performant to use '\n'
There was a problem hiding this comment.
It is a performance warning from clang-tidy. I'm not sure it impacts heavily. std::endl forces a flush but \n doesn't so it might optimize the performance of stream output slightly
There was a problem hiding this comment.
Oh I didn't know that. But for logs, don't we want to flush the buffer to make sure the log is written?
There was a problem hiding this comment.
Actually it will flush but after some time, just not immediately so it can write multiple lines at once.
There was a problem hiding this comment.
Doesn't #1639 only change the std::endl in the logger? If so, that is the same function as now. The change probably got lost when I refactored the logger.
I do think that the std::endls sprinkled in examples (mainly the help strings) need to be changed to '\n' tho.
There was a problem hiding this comment.
Yes you are right, all of them were Logger or TablePrinter. But my question is should I suppress the warning completely from all files in clang-tidy or just very simple cases like this?
In examples there are some cases just multiline strings are printed with std::endl in printUsage() or EXIT_WITH_ERROR(). For printUsage I think it should be definitely \n but what should be done for error macros? One thing in my mind use std::endl for just error strings and in last line of complex multiline strings to ensure it is flushed in last line. But it will require many inline suppressions
There was a problem hiding this comment.
In examples there are some cases just multiline strings are printed with
std::endlinprintUsage()orEXIT_WITH_ERROR(). ForprintUsageI think it should be definitely\nbut what should be done for error macros? One thing in my mind usestd::endlfor just error strings and in last line of complex multiline strings to ensure it is flushed in last line. But it will require many inline suppressions
I think using std::endl at the end of a conceptual message is a good solution.
There was a problem hiding this comment.
Maybe it's better to just suppress this error and fix whatever we think needs fixing?
There was a problem hiding this comment.
@seladb Ok agreed I'll add global suppression for it. We can discuss it for Examples more precisely (which should keep or replaced) in its PR. Anyway, I already reverted in this PR, so it is ready to merge if there is no anything different than this
Because of the slow transition and fast changes in Common++ new warnings are generated by clang-tidy in Common++. This PR fixes new issues.
Part of #1675