[CODE HEALTH] Resolve multiple clang tidy warnings - #4492
Conversation
dbarker
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4492 +/- ##
==========================================
- Coverage 83.18% 83.17% -0.00%
==========================================
Files 520 520
Lines 20321 20321
==========================================
- Hits 16901 16899 -2
- Misses 3420 3422 +2
🚀 New features to boost your workflow:
|
denizariyan
left a comment
denizariyan
left a comment
There was a problem hiding this comment.
Nice cleanup, thanks!
| { | ||
| std::array<int, 1000> batch{}; | ||
| std::default_random_engine generator; | ||
| std::default_random_engine generator{std::random_device{}()}; |
There was a problem hiding this comment.
I assume this is from a bugprone-random-generator-seed right? That rule is more about security related RNG which needs to be random while a benchmark usually wants to not be random.
With actual randomness, the benchmarks would have non-reproducible input which we should avoid so that a before/after comparison doesn't have a potential to be affected by the input unintentionally.
For this case ideally in a benchmark we:
- Don't use
default_random_engineas it's implementation defined and differs across implementations - Use an explicitly set seed instead of relying on implementation defaults or an actual random device
The first point is not new now so I think we could keep it for now but I think we should not use a random device now. Applies similarly to other random usage changes in this PR. WDYT?
e.g:
| std::default_random_engine generator{std::random_device{}()}; | |
| // NOLINTNEXTLINE(bugprone-random-generator-seed): keep input reproducible | |
| std::default_random_engine generator{42}; |
There was a problem hiding this comment.
Great analysis. Yes a fixed seed would be better. As you noted there is some room for improvement throughout the benchmarks outside this PR. Please share if you find any other cases.
There was a problem hiding this comment.
Thanks for the suggestion, I also agree a fixed seed is better for test code.
| const std::string service_name = detail::GetServiceName(); | ||
| const std::string unknown_service_prefix = "unknown_service:"; | ||
| if (service_name.substr(0, unknown_service_prefix.size()) == unknown_service_prefix) | ||
| { | ||
| EXPECT_GT(service_name.size(), std::string{"unknown_service:"}.size()); | ||
| EXPECT_GT(service_name.size(), unknown_service_prefix.size()); |
There was a problem hiding this comment.
nit: is this due to abseil-string-find-startswith? Maybe we could just disable the rule similar to abseil-string-find-str-contains since the rule suggests using abseil helpers which we don't have here?
There was a problem hiding this comment.
Good feedback! Yes I think it is better to disable the abseil-string-find-startswith check. This check will come back in the modernize form once building with c++20
marcalff
left a comment
marcalff
left a comment
There was a problem hiding this comment.
LGTM, thanks for the cleanup.
See suggestions from @denizariyan on fixed seeds in benchmarks.
3 participants
Contributes to #2053
Changes
resolves the following warnings:
modernize-use-using- 8 warningscppcoreguidelines-use-enum-class- 7 warningsbugprone-random-generator-seed- 5 warningsmodernize-deprecated-headers- 4 warningsmisc-predictable-rand- 3 warningsbugprone-derived-method-shadowing-base-method- 3 warningsbugprone-unintended-char-ostream-output- 2 warningsperformance-move-const-arg- 2 warningscppcoreguidelines-macro-to-enum- 2 warningsabseil-string-find-startswith- 1 warningmisc-use-internal-linkage- 1 warningFor significant contributions please make sure you have completed the following items:
CHANGELOG.mdupdated for non-trivial changes