[clang-tidy] extend misc-misplaced-const to detect using besides typedef

[AlexanderLanin]

looking at this at least many are indeed valid results:

partial-misc-misplaced-const-llvm-output.zip140 KBDownload

I guess false positives could be reduced by eliminating those cases where the variables are (intentionally) modified.

FIX-IT isn't quite that obvious. Some options:

• look for other typedef which contains the same as const
• create new typedef/using
• remove "*" from typedef and adjust all usage accordingly. Implies removing "Ptr" suffix.

clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.cpp

2

is this good practice? I didn't want to duplicate everything here.

32

not sure how to remove the regex here without duplicating everything

[Eugene.Zelenko]

clang-tools-extra/clang-tidy/misc/MisplacedConstCheck.cpp

64

llvm_unreachable

[AlexanderLanin]

Updated revision with llvm_unreachable

[aaron.ballman]

LGTM!

In D72373#1809080, @AlexanderLanin wrote:

FIX-IT isn't quite that obvious. Some options:

• look for other typedef which contains the same as const
• create new typedef/using
• remove "*" from typedef and adjust all usage accordingly. Implies removing "Ptr" suffix.

I think we can punt on the fixit for now as this is a good incremental improvement. Another possible option is to remove the const from the declaration using the typedef (there may be situations where you cannot add a new typedef or modify the existing one because the typedef is in a system header).

clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.cpp

2

Yes, this is a good way to reduce duplication.

32

I think the regex is reasonable enough.

[AlexanderLanin]

Thanks for the review!

Could someone commit this? As I can not.
Alexander Lanin <[e-mail]>

[AlexanderLanin]

ping

Could someone commit this? As I can not.
Alexander Lanin <[e-mail]>

[aaron.ballman]

I've commit on your behalf in ecc7dae50c41bc8a129a158ecf0ae0270126505c. Sorry about the delay in committing and thank you for the patch!

[thakis]

Looks like this breaks tests: http://45.33.8.238/linux/8102/step_8.txt

[aaron.ballman]

In D72373#1833569, @thakis wrote:

Looks like this breaks tests: http://45.33.8.238/linux/8102/step_8.txt

I reverted in e3b15ed376f3753d2a4e16281f8230e4ffed41ba. @AlexanderLanin, do you mind taking a look?

[AlexanderLanin]

Updated misc-misplaced-const.c to reflect new output and fixed one wrong col in misc-misplaced-const.cpp - I really do not understand how that happened.
Rebased and verified with check-clang-tools.

[aaron.ballman]

In D72373#1834591, @AlexanderLanin wrote:

Updated misc-misplaced-const.c to reflect new output and fixed one wrong col in misc-misplaced-const.cpp - I really do not understand how that happened.
Rebased and verified with check-clang-tools.

Thanks! I've committed on your behalf in 84c5f196370065388779cd96d033c84d31031543

clang-tools-extra/clang-tidy/misc/MisplacedConstCheck.cpp

 
 using namespace clang::ast_matchers;
 
 namespace clang {
 namespace tidy {
 namespace misc {
 
 void MisplacedConstCheck::registerMatchers(MatchFinder *Finder) {
+  auto NonConstAndNonFunctionPointerType = hasType(pointerType(unless(
+      pointee(anyOf(isConstQualified(), ignoringParens(functionType()))))));
+
   Finder->addMatcher(
-      valueDecl(hasType(isConstQualified()),
-                hasType(typedefType(hasDeclaration(
-                    typedefDecl(hasType(pointerType(unless(pointee(
-                                    anyOf(isConstQualified(),
-                                          ignoringParens(functionType())))))))
-                        .bind("typedef")))))
+      valueDecl(
+          hasType(isConstQualified()),
+          hasType(typedefType(hasDeclaration(anyOf(
+              typedefDecl(NonConstAndNonFunctionPointerType).bind("typedef"),
+              typeAliasDecl(NonConstAndNonFunctionPointerType)
+                  .bind("typeAlias"))))))
           .bind("decl"),
       this);
 }
 
 static QualType guessAlternateQualification(ASTContext &Context, QualType QT) {
   // We're given a QualType from a typedef where the qualifiers apply to the
   // pointer instead of the pointee. Strip the const qualifier from the pointer
   // type and add it to the pointee instead.
   if (!QT->isPointerType())
     return QT;
 
   Qualifiers Quals = QT.getLocalQualifiers();
   Quals.removeConst();
 
   QualType NewQT = Context.getPointerType(
       QualType(QT->getPointeeType().getTypePtr(), Qualifiers::Const));
   return NewQT.withCVRQualifiers(Quals.getCVRQualifiers());
 }
 
 void MisplacedConstCheck::check(const MatchFinder::MatchResult &Result) {
   const auto *Var = Result.Nodes.getNodeAs<ValueDecl>("decl");
-  const auto *Typedef = Result.Nodes.getNodeAs<TypedefDecl>("typedef");
   ASTContext &Ctx = *Result.Context;
   QualType CanQT = Var->getType().getCanonicalType();
 
-  diag(Var->getLocation(), "%0 declared with a const-qualified typedef type; "
-                           "results in the type being '%1' instead of '%2'")
-      << Var << CanQT.getAsString(Ctx.getPrintingPolicy())
+  SourceLocation AliasLoc;
+  const char *AliasType;
+  if (const auto *Typedef = Result.Nodes.getNodeAs<TypedefDecl>("typedef")) {
+    AliasLoc = Typedef->getLocation();
+    AliasType = "typedef";
+  } else if (const auto *TypeAlias =
+                 Result.Nodes.getNodeAs<TypeAliasDecl>("typeAlias")) {
+    AliasLoc = TypeAlias->getLocation();
+    AliasType = "type alias";
+  } else {
+    assert(0 && "This should not be reachable");
+  }
+
+  diag(Var->getLocation(), "%0 declared with a const-qualified %1; "
+                           "results in the type being '%2' instead of '%3'")
+      << Var << AliasType << CanQT.getAsString(Ctx.getPrintingPolicy())
       << guessAlternateQualification(Ctx, CanQT)
              .getAsString(Ctx.getPrintingPolicy());
-  diag(Typedef->getLocation(), "typedef declared here", DiagnosticIDs::Note);
+  diag(AliasLoc, "%0 declared here", DiagnosticIDs::Note) << AliasType;
 }
 
 } // namespace misc
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/clang-tidy/checks/misc-misplaced-const.rst

 .. title:: clang-tidy - misc-misplaced-const
 
 misc-misplaced-const
 ====================
 
-This check diagnoses when a ``const`` qualifier is applied to a ``typedef`` to a
-pointer type rather than to the pointee, because such constructs are often
-misleading to developers because the ``const`` applies to the pointer rather
-than the pointee.
+This check diagnoses when a ``const`` qualifier is applied to a ``typedef``/
+``using`` to a pointer type rather than to the pointee, because such constructs
+are often misleading to developers because the ``const`` applies to the pointer
+rather than the pointee.
 
 For instance, in the following code, the resulting type is ``int *`` ``const``
 rather than ``const int *``:
 
 .. code-block:: c++
 
   typedef int *int_ptr;
-  void f(const int_ptr ptr);
-
-The check does not diagnose when the underlying ``typedef`` type is a pointer to
-a ``const`` type or a function pointer type. This is because the ``const``
-qualifier is less likely to be mistaken because it would be redundant (or
-disallowed) on the underlying pointee type.
+  void f(const int_ptr ptr) {
+    *ptr = 0; // potentially quite unexpectedly the int can be modified here
+    ptr = 0; // does not compile
+  }
+
+The check does not diagnose when the underlying ``typedef``/``using`` type is a
+pointer to a ``const`` type or a function pointer type. This is because the
+``const`` qualifier is less likely to be mistaken because it would be redundant
+(or disallowed) on the underlying pointee type.

clang-tools-extra/test/clang-tidy/checkers/misc-misplaced-const.cpp

-// RUN: %check_clang_tidy %s misc-misplaced-const %t
+// RUN: %check_clang_tidy %s misc-misplaced-const %t -- -- -DUSING
+// RUN: %check_clang_tidy %s misc-misplaced-const %t -- -- -DTYPEDEF
 
-typedef int plain_i;
-typedef int *ip;
-typedef const int *cip;
-
-void func() {
-  if (const int *i = 0)
-    ;
-  if (const plain_i *i = 0)
-    ;
-  if (const cip i = 0)
-    ;
-
-  // CHECK-MESSAGES: :[[@LINE+1]]:16: warning: 'i' declared with a const-qualified typedef type; results in the type being 'int *const' instead of 'const int *'
-  if (const ip i = 0)
-    ;
+#ifdef TYPEDEF
+typedef int int_;
+typedef int *ptr_to_int;
+typedef const int *ptr_to_const_int;
+#endif
+#ifdef USING
+using int_ = int;
+using ptr_to_int = int *;
+using ptr_to_const_int = const int *;
+#endif
+
+void const_pointers() {
+  if (const int *i = 0) {
+    i = 0;
+    // *i = 0;
+  }
+
+  if (const int_ *i = 0) {
+    i = 0;
+    // *i = 0;
+  }
+
+  if (const ptr_to_const_int i = 0) {
+    // i = 0;
+    // *i = 0;
+  }
+
+  // Potentially quite unexpectedly the int can be modified here
+  // CHECK-MESSAGES: :[[@LINE+1]]:23: warning: 'i' declared with a const-qualified {{.*}}; results in the type being 'int *const' instead of 'const int *'
+  if (const ptr_to_int i = 0) {
+    //i = 0;
+
+    *i = 0;
+  }
 }
 
 template <typename Ty>
 struct S {
   const Ty *i;
   const Ty &i2;
 };
 
 template struct S<int>;
-template struct S<ip>; // ok
-template struct S<cip>;
+template struct S<ptr_to_int>; // ok
+template struct S<ptr_to_const_int>;
 
 template <typename Ty>
 struct U {
   const Ty *i;
   const Ty &i2;
 };
 
 template struct U<int *>; // ok
 
 struct T {
   typedef void (T::*PMF)();
 
   void f() {
     const PMF val = &T::f; // ok
   }
 };