[clang-tidy] Fix `readability-non-const-parameter` for parameter referenced by an lvalue

[Sockke]

Summary

The checker missed a check for a case when the parameter is referenced by an lvalue and this could cause build breakages.

[MTC]

clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp

86–99

86~99 is pretty close to 64~81, could you please refactor it?

[Sockke]

clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp

86–99

86~99 is pretty close to 64~81, could you please refactor it?

FunctionDecl and CXXConstructDecl have different methods with the same name.

[aaron.ballman]

LGTM with some small nits, but can you also add a release note for the fix? (If there was a bug report for this issue, you should mention that report in the release note and close the bug out.)

clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp

292

Please spell out enough of the warning you don't expect to see (in all of these cases) so that someone reading the code knows what you expect to miss.

[Sockke]

Add a release note and improve test description information.

[MTC]

clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp

293

@Sockke Could you please add the following tests?

int &ref = std::ref(*p);
const int &cref = std::ref(*p);
const int &cref = std::cref(*p);
const int *ptr = std::as_const(p);
int *ptr = const_cast<int*>(std::as_const(p));
decltype(auto) ptr = p;
auto ptr = p;

[Sockke]

clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp

293

These cases have been handled stably in another logic.

[MTC]

clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp

293

Sorry for my mistake, look good to me!

[njames93]

Before landing make sure this is rebased against trunk, just so the pre-merge bot can run and verify there are no issues. It looks like this patch is based off a few commits that don't appear on trunk.

[Sockke]

rebased.

clang-tools-extra/clang-tidy/readability/NonConstParameterCheck.cpp

if (const auto *B = dyn_cast<BinaryOperator>(S)) {
             continue;
           markCanNotBeConst(Arg->IgnoreParenCasts(), false);
         }
       }
     } else if (const auto *CE = dyn_cast<CXXConstructExpr>(S)) {
       for (const auto *Arg : CE->arguments()) {
         markCanNotBeConst(Arg->IgnoreParenCasts(), true);
       }
+      // Data passed by nonconst reference should not be made const.
+      unsigned ArgNr = 0U;
+      if (const auto *CD = CE->getConstructor()) {
+        for (const auto *Par : CD->parameters()) {
+          if (ArgNr >= CE->getNumArgs())
+            break;
+          const Expr *Arg = CE->getArg(ArgNr++);
+          // Is this a non constant reference parameter?
+          const Type *ParType = Par->getType().getTypePtr();
+          if (!ParType->isReferenceType() || Par->getType().isConstQualified())
+            continue;
+          markCanNotBeConst(Arg->IgnoreParenCasts(), false);
+        }
+      }
     } else if (const auto *R = dyn_cast<ReturnStmt>(S)) {
       markCanNotBeConst(R->getRetValue(), true);
     } else if (const auto *U = dyn_cast<UnaryOperator>(S)) {
       markCanNotBeConst(U, true);
     }
   } else if (const auto *VD = Result.Nodes.getNodeAs<VarDecl>("Mark")) {
     const QualType T = VD->getType();
     if ((T->isPointerType() && !T->getPointeeType().isConstQualified()) ||
         T->isArrayType())
       markCanNotBeConst(VD->getInit(), true);
+    else if (T->isLValueReferenceType() &&
+             !T->getPointeeType().isConstQualified())
+      markCanNotBeConst(VD->getInit(), false);
   }
 }
 
 void NonConstParameterCheck::addParm(const ParmVarDecl *Parm) {
   // Only add nonconst integer/float pointer parameters.
   const QualType T = Parm->getType();
   if (!T->isPointerType() || T->getPointeeType().isConstQualified() ||
       !(T->getPointeeType()->isIntegerType() ||

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`bugprone-shared-ptr-array-mismatch <clang-tidy/checks/bugprone-shared-ptr-array-mismatch>` check.
   Finds initializations of C++ shared pointers to non-array type that are initialized with an array.
 
 New check aliases
 ^^^^^^^^^^^^^^^^^
 
 Changes in existing checks
 ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
+- Fixed a false positive in :doc:`readability-non-const-parameter
+  <clang-tidy/checks/readability-non-const-parameter>` when the parameter is referenced by an lvalue
+
+
 Removed checks
 ^^^^^^^^^^^^^^
 
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 

clang-tools-extra/docs/clang-tidy/checks/readability-non-const-parameter.rst

.. code-block:: c++
 
   // no warning; Technically, *p can be const ("const struct S *p"). But making
   // *p const could be misleading. People might think that it's safe to pass
   // const data to this function.
   struct S { int *a; int *b; };
   int f3(struct S *p) {
     *(p->a) = 0;
   }
+
+  // no warning; p is referenced by an lvalue.
+  void f4(int *p) {
+    int &x = *p;
+  }

clang-tools-extra/test/clang-tidy/checkers/readability-non-const-parameter.cpp

 // CHECK-FIXES: {{^}}extern char foo(const char *s); // 1{{$}}
 // CHECK-MESSAGES: :[[@LINE+1]]:16: warning: pointer parameter 's' can be
 char foo(char *s) {
   // CHECK-FIXES: {{^}}char foo(const char *s) {{{$}}
   return *s;
 }
 char foo(char *s); // 2
 // CHECK-FIXES: {{^}}char foo(const char *s); // 2{{$}}
+
+void lvalueReference(int *p) {
+  // CHECK-MESSAGES-NOT: warning: pointer parameter 'p' can be
+  int &x = *p;
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:32: warning: pointer parameter 'p' can be
+void constLValueReference(int *p) {
+  // CHECK-FIXES: {{^}}void constLValueReference(const int *p) {{{$}}
+  const int &x = *p;
+}
+
+void lambdaLVRef(int *p) {
+  // CHECK-MESSAGES-NOT: warning: pointer parameter 'p' can be
+  auto foo = [&]() {
+    int &x = *p;
+  };
+}
+
+// CHECK-MESSAGES: :[[@LINE+1]]:28: warning: pointer parameter 'p' can be
+void lambdaConstLVRef(int *p) {
+  // CHECK-FIXES: {{^}}void lambdaConstLVRef(const int *p) {{{$}}
+  auto foo = [&]() {
+    const int &x = *p;
+  };
+}
+
+struct Temp1 {
+  Temp1(int &i) {
+    i = 10;
+  }
+};
+void constructLVRef(int *p) {
+  // CHECK-MESSAGES-NOT: warning: pointer parameter 'p' can be
+  Temp1 t(*p);
+}