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

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, enhancement)

Description

[Andi [:andi]]

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.

Comment 1

[Andi [:andi]]

Attachment: Bug 1471285 - [Clang-Tidy 5.0.1] Checker misc-suspicious-missing-comma has faulty test case.

Review commit: https://reviewboard.mozilla.org/r/253174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/253174/

Comment 2

[Sylvestre Ledru [:Sylvestre]]

what was wrong?

Comment 3

[Andi [:andi]]

(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

Comment 4

[Andi [:andi]]

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 5

[Jan Keromnes [:janx]]

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!

Comment 6

[Jan Keromnes [:janx]]

Please send a try push with the job "static-analysis-autotest-linux64-st-autotest/debug" (or just "autotest" with `./mach try fuzzy`).

Comment 7

[Andi [:andi]]

(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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/77cadf8638c6
[Clang-Tidy 5.0.1] Checker misc-suspicious-missing-comma has faulty test case. r=janx

Comment 9

[Andi [:andi]]

Everything is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf5f4cd6162a523eed06c3592948b5b230b23f3e

Comment 10

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/77cadf8638c6