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

[Nathan James via Phabricator via cfe-commits]

njames93 added a subscriber: sammccall.
njames93 added a comment.

In D54943#3296191 <https://reviews.llvm.org/D54943#3296191>, @JonasToth wrote:

> In D54943#3292552 <https://reviews.llvm.org/D54943#3292552>, @0x8000-0000 wrote:
>
>> @aaron.ballman - can this land for Clang14, or does it have wait for 15?
>>
>> Are there any other reviewers that can approve this?
>
> Sadly, cherry-picking to 14 is very unlikely, as this is not a bugfix. :/
> On the other hand, it missed so many releases so lets stay positive :)
>
> Aaron did the review and I think he should accept too.

This won't get backported now, besides giving it a few months to test(and potentially iron out) on the dev branch isn't such a bad thing.



================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt:33
   LINK_LIBS
+  clangAnalysis
   clangTidy
----------------
Will this work under clangd as I don't think that links in the clangAnalysis libraries @sammccall?


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:27
+AST_MATCHER(VarDecl, isLocal) { return Node.isLocalVarDecl(); }
+AST_MATCHER_P(DeclStmt, containsDeclaration2,
+              ast_matchers::internal::Matcher<Decl>, InnerMatcher) {
----------------
nit


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:84
+              compoundStmt(
+                  findAll(declStmt(allOf(containsDeclaration2(
+                                             LocalValDecl.bind("local-value")),
----------------
`allOf` is unnecessary here as `declStmt` is implicitly an `allOf` matcher.
Also `findAll` isn't the right matcher in this case, you likely want `forEachDescendant`. `findAll` will try and match on the `CompoundStmt` which is always going to fail.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:102-105
+  // There have been crashes on 'Variable == nullptr', even though the matcher
+  // is not conditional. This comes probably from 'findAll'-matching.
+  if (!Variable || !LocalScope)
+    return;
----------------
This sounds like a bug that should be raised with ASTMatchers, if its reproducible.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:160
+  using namespace utils::fixit;
+  using llvm::Optional;
+  if (VC == VariableCategory::Value && TransformValues) {
----------------
This is unnecessary, `llvm::Optional` already has an alias inside the clang namespace.


================
Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:162-168
+    if (Optional<FixItHint> Fix = addQualifierToVarDecl(
+            *Variable, *Result.Context, DeclSpec::TQ_const,
+            QualifierTarget::Value, QualifierPolicy::Right)) {
+      Diag << *Fix;
+      // FIXME: Add '{}' for default initialization if no user-defined default
+      // constructor exists and there is no initializer.
+    }
----------------
DiagnosticBuilders accept optional fixits as syntactic sugar.
Same goes for below.


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:108-112
+- New :doc:`cppcoreguidelines-const-correctness
+  <clang-tidy/checks/cppcoreguidelines-const-correctness>` check.
+
+  Detects unmodified local variables and suggest adding ``const`` if the transformation is possible.
+
----------------
This'll need rebasing after branching.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp:3-6
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformReferences', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
----------------



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp:3-5
+// RUN:  [{key: "cppcoreguidelines-const-correctness.AnalyzeValues", value: 1},\
+// RUN:   {key: "cppcoreguidelines-const-correctness.WarnPointersAsValues", value: 1}, \
+// RUN:   {key: "cppcoreguidelines-const-correctness.TransformPointersAsValues", value: 1},\
----------------



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp:3-5
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1},\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
----------------



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-unaligned.cpp:3-5
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
----------------



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:3-5
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943