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

[omtcyfz]

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.

[Eugene.Zelenko]

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

[Eugene.Zelenko]

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp

27

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

[malcolm.parsons]

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp

31

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

[omtcyfz]

Address couple comments.

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp

27

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

31

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

[Eugene.Zelenko]

docs/ReleaseNotes.rst

62

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

[omtcyfz]

Minor improvements.

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp

31

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

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

[ioeric]

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

[omtcyfz]

In D25024#557033, @ioeric wrote:

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]

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]

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]

Addressed bunch of comments.

[omtcyfz]

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.

[malcolm.parsons]

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]

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]

Opened an issue in CppCoreGuidelines Github repo.

[malcolm.parsons]

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

Declaration	CERT	CppCoreGuidelines	Me
int i;	GOOD	GOOD	GOOD
int i = 1;	GOOD	GOOD	GOOD
int *p;	GOOD	GOOD	GOOD
int *p, q;	BAD	BAD	BAD
int i, j;	GOOD	GOOD	GOOD
int i, j = 1;	BAD	GOOD	BAD
int i = 1, j = 1;	BAD	GOOD	GOOD
int i = 1, j;	BAD	GOOD	BAD
int *i, *j;	BAD	BAD	BAD
for (int i = 0, j = size; i != j; i++)	GOOD	GOOD	GOOD
for (int *p = a, *q = a+size; p != q; p++)	GOOD	BAD	GOOD

I agree with CERT for most cases.

17

and global variables.

[alexfh]

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

[omtcyfz]

No longer relevant.