[PATCH] D54943: [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked an inline comment as done.
JonasToth added a comment.

addressed all outstanding review comments.



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp:107-108
+  int *np_local3[2] = {&np_local0[0], &np_local0[1]};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local3' of type 'int *[2]' can be declared 'const'
+  // CHECK-FIXES: const
+  for (int *non_const_ptr : np_local3) {
----------------
aaron.ballman wrote:
> This is a false positive. Making `np_local3` be `const` will break the code: https://godbolt.org/z/EExKfsW4G
Thanks for spotting this!
The analyzer did not consider the `int * non_const_ptr: np_local3` to be a mutation, as its not a reference type.

```
  444   // The range variable is a reference to a builtin array. In that case the
    1   // array is considered **modified if the loop-variable is a non-const reference**.
    2   const auto DeclStmtToNonRefToArray = declStmt(hasSingleDecl(varDecl(hasType(
    3       hasUnqualifiedDesugaredType(referenceType(pointee(arrayType())))))));
    4   const auto RefToArrayRefToElements = match(
    5       findAll(stmt(cxxForRangeStmt(
    6                        hasLoopVariable(varDecl(hasType(nonConstReferenceType()))
    7                                            .bind(NodeID<Decl>::value)),
    8                        hasRangeStmt(DeclStmtToNonRefToArray),
    9                        hasRangeInit(canResolveToExpr(equalsNode(Exp)))))
   10                   .bind("stmt")),
   11       Stm, Context);
 ```

Added the fix to the patch, is that ok with you?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:17
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
----------------
aaron.ballman wrote:
> Similarly, showing anonymous namespace variables would help, as those are like a static global and can also theoretically known to be const. (Note, I don't insist on catching those cases! Just would like test coverage with some comments on what to expect + docs if it seems important to tell users.)
Globals are not matched in any way. But there is `cppcoreguidelines-avoid-non-const-global-variables` which warns for non-const globals already.

The docs say `This check implements detection of local variables which could be declared as`, so its implicit that globals are not caught. I think that its enough. I will add a reference to `cppcoreguidelines-avoid-non-const-global-variables` though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54943/new/

https://reviews.llvm.org/D54943