Page MenuHomePhabricator

[clang-tidy] Add check for detecting declarations with multiple names
AbandonedPublic

Authored by omtcyfz on Sep 28 2016, 8:06 AM.

Details

Reviewers
None
Summary

C++ Core Guidelines Section "Expressions and statements" Rule 10 suggests to split declarations with multiple names into multiple single declarations. See the following example.

Example, bad.

std::vector<int> velocities(10, 0), numbers(other_numbers),
                          inputs(input.begin(), input.end());

Example, good.

std::vector<int> velocities(10, 0);
std::vector<int> numbers(other_numbers);
std::vector<int> inputs(input.begin(), input.end());

http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#a-nameres-name-oneaes10-declare-one-name-only-per-declaration

Currently stalled, removed everyone from subscribers and reviewers for convenience.

Diff Detail

Event Timeline

omtcyfz updated this revision to Diff 72830.Sep 28 2016, 8:06 AM
omtcyfz retitled this revision from to [clang-tidy] Add check for detecting declarations with multiple names.
omtcyfz updated this object.
omtcyfz added reviewers: alexfh, aaron.ballman, ioeric.
omtcyfz added a subscriber: cfe-commits.
omtcyfz updated this object.Sep 28 2016, 8:07 AM

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp
26

Namespace is inconsistent with following ones. It I'm not mistaken, // namespace is prevailing form in CLang-tidy code.

malcolm.parsons added inline comments.
clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp
30

Can declCountIsGreaterThan(1) be written as unless(declCountIs(1))?

omtcyfz updated this revision to Diff 72881.Sep 28 2016, 12:45 PM
omtcyfz marked an inline comment as done.

Address couple comments.

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp
26

You're right. That's interesting though, because LLVM Coding Guidelines suggest end namespace mah_name.

30

Sure it can. I just thought declCountIsGreaterThan might be useful at some point.

docs/ReleaseNotes.rst
62 ↗(On Diff #72881)

Please add one sentence description. Probably Checks for declarations with multiple names. should be enough.

omtcyfz updated this revision to Diff 73025.Sep 30 2016, 3:16 AM
omtcyfz marked an inline comment as done.

Minor improvements.

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp
30

Actually, for the sake of simplicity I'll use unless(declCountIs(1)), when some other part of code would need similar stuff we'd add it. For now it's only one use case, so I should be fine.

Thank you very much for the suggestion!

docs/ReleaseNotes.rst
62 ↗(On Diff #72881)

Aw, yes, totally forgot about that. Thank you very much!

ioeric removed a reviewer: ioeric.Sep 30 2016, 5:34 AM
ioeric added a subscriber: ioeric.

The code looks about right, but I am not the right person to decide whether checks go into clang-tidy.

The code looks about right, but I am not the right person to decide whether checks go into clang-tidy.

Okay, thank you!

aaron.ballman added inline comments.Oct 3 2016, 1:36 PM
clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
40

Can you also register this check under the name cert-dcl04-c as well? It fits the behavior of that recommendation too (aside from the exceptions in the recommendation, but those can be handled later). This will also require adding a .rst file for that check name and having it redirect appropriately.

https://www.securecoding.cert.org/confluence/display/c/DCL04-C.+Do+not+declare+more+than+one+variable+per+declaration

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp
28

s/diagnostics/fixit ?

39

Diagnostics do not start with a capital letter. Also, this diagnostic is not really accurate. Consider void f(int x, int y); -- that's a single declaration, but it declares multiple names. Perhaps: do not declare more than one variable per declaration since this should really be focusing on variable declarations rather than parameters, template parameter lists, etc

test/clang-tidy/cppcoreguidelines-one-name-per-declaration.cpp
17

Please add tests that show the exceptions in the C++ Core Guidelines are properly handled. Also, I'd like to see tests with other named declarations, such as typedefs, template parameter lists, for loop initializers, etc.

malcolm.parsons added inline comments.Oct 3 2016, 2:14 PM
test/clang-tidy/cppcoreguidelines-one-name-per-declaration.cpp
9

The guideline says "Flag non-function arguments with multiple declarators involving declarator operators (e.g., int* p, q;)".

There are no declarator operators in this test, so there should be no warning.

17

and structured bindings (no warning).

omtcyfz updated this revision to Diff 74509.Oct 13 2016, 7:01 AM
omtcyfz marked 5 inline comments as done.

Addressed bunch of comments.

omtcyfz added inline comments.Oct 13 2016, 7:01 AM
clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp
39

Diagnostics do not start with a capital letter.

Clang SA diags do, actually. Though I can totally see the reason: consistency is important since it's clang-tidy check.

Consider void f(int x, int y); -- that's a single declaration, but it declares multiple names. Perhaps: do not declare more than one variable per declaration since this should really be focusing on variable declarations rather than parameters, template parameter lists, etc

Fixed, thank you for the note!

test/clang-tidy/cppcoreguidelines-one-name-per-declaration.cpp
9

The guideline says

Reason: One-declaration-per line increases readability and avoids mistakes related to the C/C++ grammar. It also leaves room for a more descriptive end-of-line comment.

Exception: a function declaration can contain several function argument declarations.

I'm not sure why what you copied is written in "Enforcement" section, but I do not think that is how it should be handled. I am concerned not only about that specific case and I see no reason to cut off cases already presented in this test.

test/clang-tidy/cppcoreguidelines-one-name-per-declaration.cpp
9

"mistakes related to the C/C++ grammar" only occur when declarator operators are involved. e.g. in int* p, q a reader might incorrectly think that q was a pointer.

I see reasons not to warn about cases like
for (auto i = c.begin(), e = someExpensiveFn(); i != e; i++)
for (int i = 0, j = someExpensiveFn(); i < j; i++);
because the alternatives increase variable scope, or for
int x = 42, y = 43;
because it's not difficult to read.

As we disagree on this, can it be made configurable?

aaron.ballman added inline comments.Oct 13 2016, 9:52 AM
test/clang-tidy/cppcoreguidelines-one-name-per-declaration.cpp
9

We usually try to ensure that the check matches the behavior required by the normative wording of coding rules we follow. Based on that, the C++ core guideline rule only cares about declarator operators while the CERT recommendation kind of does not care about them. If you think the C++ core guideline enforcement is wrong, you can file a bug against that project to see if the editors agree, but I think the check should match the documented behavior from the guidelines. FWIW, I'm happy to work on the CERT semantics if you don't want to deal with those beyond what you've already done (or you can tackle the semantic differences if you want).

omtcyfz added a comment.EditedOct 18 2016, 4:36 AM

Opened an issue in CppCoreGuidelines Github repo.

test/clang-tidy/cppcoreguidelines-one-name-per-declaration.cpp
9

The CERT recommendation does care about declarator operators:

"Multiple, simple variable declarations can be declared on the same line given that there are no initializations. A simple variable declaration is one that is not a pointer or array."

DeclarationCERTCppCoreGuidelinesMe
int i;GOODGOODGOOD
int i = 1;GOODGOODGOOD
int *p;GOODGOODGOOD
int *p, q;BADBADBAD
int i, j;GOODGOODGOOD
int i, j = 1;BADGOODBAD
int i = 1, j = 1;BADGOODGOOD
int i = 1, j;BADGOODBAD
int *i, *j;BADBADBAD
for (int i = 0, j = size; i != j; i++)GOODGOODGOOD
for (int *p = a, *q = a+size; p != q; p++)GOODBADGOOD

I agree with CERT for most cases.

17

and global variables.

alexfh removed a reviewer: alexfh.Jan 24 2017, 10:12 AM
alexfh added a subscriber: alexfh.

https://reviews.llvm.org/D27621 seems to have a more up-to-date version of this patch.

omtcyfz edited the summary of this revision. (Show Details)Sep 21 2017, 4:36 PM
omtcyfz removed reviewers: klimek, aaron.ballman.
omtcyfz removed subscribers: alexfh, modocache, ioeric and 6 others.
omtcyfz abandoned this revision.Jul 20 2018, 1:48 AM

No longer relevant.