Fix #14206 --showtime does not account for addons #7904
Conversation
|
LGTM. As an improvement the time could also be collected for the individual addons. |
|
as a start LGTM.. I have some small nits.. like "cancell" should be "cancel". I would like that we reuse this timer for the "--showtime=file-total" output also so a different class name would be better. |
danmar
left a comment
danmar
left a comment
There was a problem hiding this comment.
If I run this command:
./cppcheck --showtime=top5_file lib/ctu.cpp lib/analyzerinfo.cpp
then I would like that the "file-total" time is shown for each file.
lib/timer.cpp
Outdated
| if (minutes.count() > 0) | ||
| ellapsedTime += std::to_string(minutes.count()) + "m "; | ||
| std::string secondsStr{std::to_string(seconds.count())}; | ||
| return (ellapsedTime + secondsStr.substr(0, secondsStr.length() - 3) + "s"); |
There was a problem hiding this comment.
It is not defined how many digits secondStr has right? So if there are 10 digits I assume the output will have 7 digits and that is way too much.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
This added new functionality but no new tests were added. Am I missing something? |
Well you are not wrong but we doesn't have anything entirly new here and in tests we mostly check for newlines for the timer output which was chaned only slightly: we don't have overall timer now for top5_file and tests modifications reflect that. As for the time measurment we can't compare it to anything as it is always different. So I agree, some small test wouldn't hurt but I suppose we already cover the changes more or less |
|
Thanks for the explanation. The problem is that it does way more than the title says. It should have probably been split into two and gotten an additional ticket for the new timer. |
3 participants
No description provided.