Page MenuHomePhabricator

[clang-tidy] Add a new checker to detect missing comma in initializer list.
ClosedPublic

Authored by etienneb on Mar 24 2016, 12:13 PM.

Details

Summary

This checker is able to detect missing comma in
an array of string literals.

const char* A[] = {
  "abc",
  "def"   // missing comma (no compiler warnings)
  "ghi",
};

The ratio of false-positive is reduced by restricting the
size of the array considered and the ratio of missing
comma.

To validate the quantity of false positive, the checker
was tried over LLVM and chromium code and detected these
cases:

http://reviews.llvm.org/D18454
https://codereview.chromium.org/1807753002/
https://codereview.chromium.org/1826193002/
https://codereview.chromium.org/1805713002/

Diff Detail

Event Timeline

etienneb updated this revision to Diff 51581.Mar 24 2016, 12:13 PM
etienneb retitled this revision from to [clang-tidy] Add a new checker to detect missing comma in initializer list..
etienneb updated this object.
etienneb added a reviewer: alexfh.
etienneb added a subscriber: cfe-commits.
etienneb updated this revision to Diff 51584.Mar 24 2016, 12:26 PM

update comment.

Hi!

I'm working on the same (or almost the same) checker as you, so maybe you'll find helpful my code.
I think your approach (or heuristics) is better than mine, but there are other options in this problem.
e.g.

  • more precise location (where exactly is the missing comma)
  • FixItHint for that
  • think about what if the explicit array size is given (see below in my test)

Please, ask me if you have any question about my checker.

(test file)

alexfh added inline comments.Mar 29 2016, 12:09 PM
clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
43

An early return would be better here.

46

Should this threshold be an option (or at least a named constant)?

53

const auto *Literal = ...

61

Should this threshold be an option (or at least a named constant)?

64

No capitalization and trailing periods, please.

Please also clang-format this file.

alexfh requested changes to this revision.Mar 29 2016, 12:11 PM
alexfh edited edge metadata.
This revision now requires changes to proceed.Mar 29 2016, 12:11 PM
etienneb updated this revision to Diff 51969.Mar 29 2016, 1:19 PM
etienneb edited edge metadata.
etienneb marked 5 inline comments as done.

address alexfh@ comments.

etienneb added inline comments.Mar 29 2016, 1:21 PM
clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
43

moved to an assert.
It should never be false.

alexfh accepted this revision.Mar 30 2016, 8:56 AM
alexfh edited edge metadata.

Looks good with a few comments. Further functionality improvements can be done in a follow up, I think.

Please update docs/ReleaseNotes.rst.

clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
73

I'd prefer this to be double(Count) / Size > RatioThreshold and RatioThreshold to be a double.

76

We need to add a recommendation on how to silence this warning (use parentheses, for example). Fine for a follow up.

This revision is now accepted and ready to land.Mar 30 2016, 8:56 AM
xazax.hun added inline comments.Mar 30 2016, 9:07 AM
clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
76

A possible way to silence this warning would be to implement a heuristic that is available in szdominik's implementation, i.e.: when the size of the array is explicitly given by the user and it matches the length of the initializer list do not warn.

alexfh added inline comments.Mar 30 2016, 9:19 AM
clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
76

The check shouldn't complain in this case, but I wouldn't recommend this as a fix, since it makes the code more strict in an inconvenient way.

Can you summarize this check in docs/ReleaseNotes.rst in the clang-tidy section, please?

etienneb updated this revision to Diff 52225.Mar 31 2016, 9:18 AM
etienneb marked 5 inline comments as done.
etienneb edited edge metadata.

address alexfh@ comments.

ready to land.

clang-tidy/misc/SuspiciousMissingCommaCheck.cpp
76

I'll provide the recommendation when it will be implemented. Next patch.

76

in case of a fixed array size, the algorithm could be more aggressive.
Feel free to push a patch for this.

I don't believe it's quite common to set the size for string literals. If you have example for it, please share it.

etienneb closed this revision.Mar 31 2016, 11:17 AM