Closed Bug 1471285 Opened 4 years ago Closed 4 years ago

[Clang-Tidy 5.0.1] Checker misc-suspicious-missing-comma has faulty test case

Categories

(Developer Infrastructure :: Source Code Analysis, enhancement)

enhancement
Not set
normal

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: andi, Assigned: andi)

Details

Attachments

(1 file)

According to the official test cases: https://github.com/llvm-mirror/clang-tools-extra/blob/release_50/test/clang-tidy/misc-suspicious-missing-comma.cpp
Looking in the implementation of the checker https://github.com/llvm-mirror/clang-tools-extra/blob/release_50/clang-tidy/misc/SuspiciousMissingCommaCheck.cpp we should use the official test cases because we use the default values for: SizeThreshold, RatioThreshold and MaxConcatenatedTokens.
what was wrong?
(In reply to Sylvestre Ledru [:sylvestre] from comment #2)
> what was wrong?

The reasoning behind this checker was that it could trigger a lot of false positives, in order to mitigate this the developer added some flags for configuration. One of them is: SizeThreshold, the default value of this is 5, so our initialiser string should have more than 5 entries in order to be valid:

https://github.com/llvm-mirror/clang-tools-extra/blob/release_50/clang-tidy/misc/SuspiciousMissingCommaCheck.cpp#L105
One note here, we should add in the future .clang-tidy to our root and customise as much as we can from the checkers that have this option.
Comment on attachment 8987884 [details]
Bug 1471285 - [Clang-Tidy 5.0.1] Checker misc-suspicious-missing-comma has faulty test case.

https://reviewboard.mozilla.org/r/253174/#review260350

Oh wow, we really had an expected result that is empty in our tests. That's bad. Thank you for fixing it!
Attachment #8987884 - Flags: review?(janx) → review+
Please send a try push with the job "static-analysis-autotest-linux64-st-autotest/debug" (or just "autotest" with `./mach try fuzzy`).
(In reply to Jan Keromnes [:janx] from comment #6)
> Please send a try push with the job
> "static-analysis-autotest-linux64-st-autotest/debug" (or just "autotest"
> with `./mach try fuzzy`).

already did this before submitting it for review ;), a couple of day back.
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/77cadf8638c6
[Clang-Tidy 5.0.1] Checker misc-suspicious-missing-comma has faulty test case. r=janx
https://hg.mozilla.org/mozilla-central/rev/77cadf8638c6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.