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

[Malcolm Parsons via cfe-commits]

malcolm.parsons added inline comments.


================
Comment at: test/clang-tidy/cppcoreguidelines-one-name-per-declaration.cpp:8
+  {
+    int x = 42, y = 43;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: Do not declare multiple names per declaration [cppcoreguidelines-one-name-per-declaration]
----------------
aaron.ballman wrote:
> malcolm.parsons wrote:
> > omtcyfz wrote:
> > > malcolm.parsons wrote:
> > > > 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.
> > > 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.
> > "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?
> 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).
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.


================
Comment at: test/clang-tidy/cppcoreguidelines-one-name-per-declaration.cpp:16
+  return 0;
+}
----------------
malcolm.parsons wrote:
> aaron.ballman wrote:
> > 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.
> and structured bindings (no warning).
and global variables.


https://reviews.llvm.org/D25024