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

clang-tidy/cert/CERTTidyModule.cpp

 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../google/UnnamedNamespaceInHeaderCheck.h"
 #include "../misc/MoveConstructorInitCheck.h"
 #include "../misc/NewDeleteOverloadsCheck.h"
 #include "../misc/NonCopyableObjects.h"
 #include "../misc/StaticAssertCheck.h"
 #include "../misc/ThrowByValueCatchByReferenceCheck.h"
+#include "../cppcoreguidelines/OneNamePerDeclarationCheck.h"
 #include "CommandProcessorCheck.h"
 #include "FloatLoopCounter.h"
 #include "SetLongJmpCheck.h"
 #include "StaticObjectExceptionCheck.h"
 #include "StrToNumCheck.h"
 #include "ThrownExceptionTypeCheck.h"
 #include "VariadicFunctionDefCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cert {
 
 class CERTModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
-    // C++ checkers
+    // C++ checks
     // DCL
     CheckFactories.registerCheck<VariadicFunctionDefCheck>(
         "cert-dcl50-cpp");
     CheckFactories.registerCheck<misc::NewDeleteOverloadsCheck>(
         "cert-dcl54-cpp");
     CheckFactories.registerCheck<google::build::UnnamedNamespaceInHeaderCheck>(
         "cert-dcl59-cpp");
     // OOP
     CheckFactories.registerCheck<misc::MoveConstructorInitCheck>(
         "cert-oop11-cpp");
     // ERR
     CheckFactories.registerCheck<SetLongJmpCheck>(
         "cert-err52-cpp");
     CheckFactories.registerCheck<StaticObjectExceptionCheck>(
         "cert-err58-cpp");
     CheckFactories.registerCheck<ThrownExceptionTypeCheck>(
         "cert-err60-cpp");
     CheckFactories.registerCheck<misc::ThrowByValueCatchByReferenceCheck>(
         "cert-err61-cpp");
 
-    // C checkers
+    // C checks
     // DCL
     CheckFactories.registerCheck<misc::StaticAssertCheck>(
         "cert-dcl03-c");
+    CheckFactories.registerCheck<cppcoreguidelines::OneNamePerDeclarationCheck>(
+        "cert-dcl04-c");
     // ENV
     CheckFactories.registerCheck<CommandProcessorCheck>(
         "cert-env33-c");
     // FLP
     CheckFactories.registerCheck<FloatLoopCounter>(
         "cert-flp30-c");
     // FIO
     CheckFactories.registerCheck<misc::NonCopyableObjectsCheck>(

clang-tidy/cert/CMakeLists.txt

add_clang_library(clangTidyCERTModule
 
   LINK_LIBS
   clangAnalysis
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
+  clangTidyCppCoreGuidelinesModule
   clangTidyGoogleModule
   clangTidyMiscModule
   clangTidyUtils
   )

clang-tidy/cppcoreguidelines/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   CppCoreGuidelinesTidyModule.cpp
   InterfacesGlobalInitCheck.cpp
+  OneNamePerDeclarationCheck.cpp
   ProBoundsArrayToPointerDecayCheck.cpp
   ProBoundsConstantArrayIndexCheck.cpp
   ProBoundsPointerArithmeticCheck.cpp
   ProTypeConstCastCheck.cpp
   ProTypeCstyleCastCheck.cpp
   ProTypeMemberInitCheck.cpp
   ProTypeReinterpretCastCheck.cpp
   ProTypeStaticCastDowncastCheck.cpp

clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 //===--- CppCoreGuidelinesModule.cpp - clang-tidy -------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "InterfacesGlobalInitCheck.h"
+#include "OneNamePerDeclarationCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
 #include "ProBoundsConstantArrayIndexCheck.h"
 #include "ProBoundsPointerArithmeticCheck.h"
 #include "ProTypeConstCastCheck.h"
 #include "ProTypeCstyleCastCheck.h"
 #include "ProTypeMemberInitCheck.h"
 #include "ProTypeReinterpretCastCheck.h"
 #include "ProTypeStaticCastDowncastCheck.h"
 #include "ProTypeUnionAccessCheck.h"
 #include "ProTypeVarargCheck.h"
 #include "SpecialMemberFunctionsCheck.h"
 #include "SlicingCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace cppcoreguidelines {
 
 /// A module containing checks of the C++ Core Guidelines
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
         "cppcoreguidelines-interfaces-global-init");
+    CheckFactories.registerCheck<OneNamePerDeclarationCheck>(
+        "cppcoreguidelines-one-name-per-declaration");
     CheckFactories.registerCheck<ProBoundsArrayToPointerDecayCheck>(
         "cppcoreguidelines-pro-bounds-array-to-pointer-decay");
     CheckFactories.registerCheck<ProBoundsConstantArrayIndexCheck>(
         "cppcoreguidelines-pro-bounds-constant-array-index");
     CheckFactories.registerCheck<ProBoundsPointerArithmeticCheck>(
         "cppcoreguidelines-pro-bounds-pointer-arithmetic");
     CheckFactories.registerCheck<ProTypeConstCastCheck>(
         "cppcoreguidelines-pro-type-const-cast");

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.h

+//===--- OneNamePerDeclarationCheck.h - clang-tidy---------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_ONE_NAME_PER_DECLARATION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_ONE_NAME_PER_DECLARATION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/// This check warns about multiple names in one declaration.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-one-name-per-declaration.html
+class OneNamePerDeclarationCheck : public ClangTidyCheck {
+public:
+  OneNamePerDeclarationCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_ONE_NAME_PER_DECLARATION_H

clang-tidy/cppcoreguidelines/OneNamePerDeclarationCheck.cpp

+//===--- OneNamePerDeclarationCheck.cpp - clang-tidy-----------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "OneNamePerDeclarationCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+namespace {
+
+AST_MATCHER_P(DeclStmt, declCountIsGreaterThan, unsigned, N) {
+  return std::distance(Node.decl_begin(), Node.decl_end()) > (ptrdiff_t)N;
+}
+
+} // namespace
+
+void OneNamePerDeclarationCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      declStmt(declCountIsGreaterThan(1)).bind("multipleNameDeclaration"),
+      this);
+}
+
+void OneNamePerDeclarationCheck::check(const MatchFinder::MatchResult &Result) {
+  // FIXME: Also provide FixItHint splitting the declarations. E.g.
+  //
+  // Before:
+  //    int x = 42, y = 43, z = 44;
+  // After:
+  //    int x = 42;
+  //    int y = 43;
+  //    int z = 44;
+  const auto *MultipleNameDeclaration =
+      Result.Nodes.getNodeAs<DeclStmt>("multipleNameDeclaration");
+  diag(MultipleNameDeclaration->getStartLoc(),
+       "do not declare more than one variable per declaration");
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang

docs/ReleaseNotes.rst

 Improvements to clang-rename
 ----------------------------
 
 - Emacs integration was added.
 
 Improvements to clang-tidy
 --------------------------
 
+- New `cppcoreguidelines-one-name-per-declaration
+  <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-one-name-per-declaration.html>`_ check
+
 - New `cppcoreguidelines-slicing
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-slicing.html>`_ check
 
   Flags slicing of member variables or vtable.
 
 - New `cppcoreguidelines-special-member-functions
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-special-member-functions.html>`_ check
 

docs/clang-tidy/checks/cppcoreguidelines-one-name-per-declaration.rst

+.. title:: clang-tidy - cppcoreguidelines-one-name-per-declaration
+
+cppcoreguidelines-one-name-per-declaration
+==========================================
+
+Checks for declarations with multiple names. C++ Core Guidelines suggests to
+split such declarations into multiple declarations each containing one name.
+
+This would improve readability and prevents potential bugs caused by inattention
+and C/C++ syntax specifics.
+
+Example, bad.
+
+.. code-block:: c++
+
+      std::vector<int> velocities(10, 0), numbers(other_numbers),
+                       inputs(input.begin(), input.end());
+
+Example, good.
+
+.. code-block:: c++
+
+      std::vector<int> velocities(10, 0);
+      std::vector<int> numbers(other_numbers);
+      std::vector<int> inputs(input.begin(), input.end());
+
+This rule is part of the "Expressions and statements" profile of the C++ Core
+Guidelines, see
+http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#a-nameres-name-oneaes10-declare-one-name-only-per-declaration

docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
    boost-use-to-string
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
+   cert-dcl04-c (redirects to cppcoreguidelines-one-name-per-declaration) <cert-dcl-04-c>
    cert-dcl50-cpp
    cert-dcl54-cpp (redirects to misc-new-delete-overloads) <cert-dcl54-cpp>
    cert-dcl59-cpp (redirects to google-build-namespaces) <cert-dcl59-cpp>
    cert-env33-c
    cert-err34-c
    cert-err52-cpp
    cert-err58-cpp
    cert-err60-cpp
    cert-err61-cpp (redirects to misc-throw-by-value-catch-by-reference) <cert-err61-cpp>
    cert-fio38-c (redirects to misc-non-copyable-objects) <cert-fio38-c>
    cert-flp30-c
    cert-oop11-cpp (redirects to misc-move-constructor-init) <cert-oop11-cpp>
    cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-one-name-per-declaration
    cppcoreguidelines-pro-bounds-array-to-pointer-decay
    cppcoreguidelines-pro-bounds-constant-array-index
    cppcoreguidelines-pro-bounds-pointer-arithmetic
    cppcoreguidelines-pro-type-const-cast
    cppcoreguidelines-pro-type-cstyle-cast
    cppcoreguidelines-pro-type-member-init
    cppcoreguidelines-pro-type-reinterpret-cast
    cppcoreguidelines-pro-type-static-cast-downcast

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

+// RUN: %check_clang_tidy %s cppcoreguidelines-one-name-per-declaration %t
+
+int main() {
+  {
+    int x = 42;
+    int y = 43;
+    // No warning here.
+  }
+  {
+    int x = 42, y = 43;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not declare more than one variable per declaration [cppcoreguidelines-one-name-per-declaration]
+  }
+  {
+    int x = 42, y = 43, z = 44;
+    // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not declare more than one variable per declaration
+  }
+  for (int i = 0, j = 100; i < j; i++);
+  // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: do not declare more than one variable per declaration
+
+  char *p, c, a[7], *pp[7], **aa[10];
+  // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: do not declare more than one variable per declaration
+  return 0;
+}
+
+void foo(int x, int y, int z) {} // Expect no warning.
+
+typedef int int_t, *intp_t, (&fp)(int, long), arr_t[10];
+
+template <typename T, typename U>
+void bar() {}
+
+template <class InputIterator, class Predicate>
+bool any_of(InputIterator first, InputIterator last, Predicate pred);