[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D54943#3854279 , @sammccall wrote:

> FYI I've sent D135829  to block this check 
> from running in clangd, after tracking it down as the cause of a >10x 
> regression in reparse times in a project that inadvertently enabled it 
> (`misc-*`).
>
> Looking at the implementation, this check seems to be expensive by design 
> rather than some bug: it's matching many nodes, and then doing expensive 
> nested analysis for each in the callback.
>
> It's reasonable to want checks like that, OTOH it would be nice to have some 
> way to distinguish them from the "classic" checks that aim to run in roughly 
> linear time.
> (It's not just clangd: we've had issues with a batch clang-tidy run over our 
> codebase not finishing due to slow checks, and takes a lot of investigation 
> before you can tell if this is a bug or not)

To quantify this: wanting to be a bit more systematic, I measured the 
performance overhead of each check in clangd by having it parse `SemaExpr.cpp` 
with 0 checks and then each check in isolation.

This check added 1600% overhead, the next highest had <30%.

Unfortunately the detailed numbers aren't reliable as it looks like some kind 
of throttling kicked in towards the end, but here they are anyway...

  I[14:53:46.703] Built preamble of size 53505060 for file 
/usr/local/google/home/sammccall/src/llvm-project/clang/lib/Sema/SemaExpr.cpp 
version null in 5.70 seconds
  I[14:53:46.708] Building AST...
  I[14:53:48.945] Indexing AST...
  I[14:53:49.049] Timing AST build with individual clang-tidy checks
  I[14:53:54.958]   Baseline = 1968848738 ns
  I[14:54:00.873]   abseil-cleanup-ctad = 0.16%
  I[14:54:06.707]   abseil-duration-addition = -1.45%
  I[14:54:12.527]   abseil-duration-comparison = -1.69%
  I[14:54:18.422]   abseil-duration-conversion-cast = -0.32%
  I[14:54:24.281]   abseil-duration-division = -0.93%
  I[14:54:30.186]   abseil-duration-factory-float = -0.06%
  I[14:54:36.107]   abseil-duration-factory-scale = 0.15%
  I[14:54:42.344]   abseil-duration-subtraction = 5.31%
  I[14:54:48.646]   abseil-duration-unnecessary-conversion = 4.86%
  I[14:54:54.617]   abseil-faster-strsplit-delimiter = 0.77%
  I[14:55:00.460]   abseil-no-internal-dependencies = -1.21%
  I[14:55:06.280]   abseil-no-namespace = -1.61%
  I[14:55:12.221]   abseil-redundant-strcat-calls = 0.04%
  I[14:55:18.138]   abseil-str-cat-append = 0.05%
  I[14:55:23.972]   abseil-string-find-startswith = -1.23%
  I[14:55:29.847]   abseil-string-find-str-contains = -0.47%
  I[14:55:35.676]   abseil-time-comparison = -1.49%
  I[14:55:41.798]   abseil-time-subtraction = 3.43%
  I[14:55:47.911]   abseil-upgrade-duration-conversions = 3.52%
  I[14:55:55.547]   altera-id-dependent-backward-branch = 29.26%
  I[14:56:01.362]   altera-kernel-name-restriction = -1.51%
  I[14:56:07.195]   altera-single-work-item-barrier = -1.38%
  I[14:56:13.013]   altera-struct-pack-align = -1.60%
  I[14:56:19.057]   altera-unroll-loops = 2.21%
  I[14:56:24.935]   android-cloexec-accept = -0.85%
  I[14:56:30.802]   android-cloexec-accept4 = -0.61%
  I[14:56:36.663]   android-cloexec-creat = -0.77%
  I[14:56:42.556]   android-cloexec-dup = -0.19%
  I[14:56:48.498]   android-cloexec-epoll-create = 0.49%
  I[14:56:54.439]   android-cloexec-epoll-create1 = 0.38%
  I[14:57:00.388]   android-cloexec-fopen = 0.67%
  I[14:57:06.329]   android-cloexec-inotify-init = 0.56%
  I[14:57:12.280]   android-cloexec-inotify-init1 = 0.25%
  I[14:57:18.201]   android-cloexec-memfd-create = 0.14%
  I[14:57:24.180]   android-cloexec-open = 1.10%
  I[14:57:30.128]   android-cloexec-pipe = 0.33%
  I[14:57:36.072]   android-cloexec-pipe2 = 0.48%
  I[14:57:41.991]   android-cloexec-socket = 0.11%
  I[14:57:47.902]   android-comparison-in-temp-failure-retry = -0.11%
  I[14:57:53.827]   boost-use-to-string = 0.23%
  I[14:57:59.841]   bugprone-argument-comment = 1.73%
  I[14:58:05.870]   bugprone-assert-side-effect = 1.98%
  I[14:58:11.787]   bugprone-assignment-in-if-condition = -0.06%
  I[14:58:17.759]   bugprone-bad-signal-to-kill-thread = 0.88%
  I[14:58:23.800]   bugprone-bool-pointer-implicit-conversion = 2.28%
  I[14:58:29.827]   bugprone-branch-clone = 1.85%
  I[14:58:35.748]   bugprone-copy-constructor-init = 0.11%
  I[14:58:41.752]   bugprone-dangling-handle = 1.61%
  I[14:58:47.659]   bugprone-dynamic-static-initializers = -0.13%
  I[14:58:53.849]   bugprone-easily-swappable-parameters = 4.48%
  I[14:58:59.756]   bugprone-exception-escape = -0.15%
  I[14:59:05.881]   bugprone-fold-init-type = 3.61%
  I[14:59:11.851]   bugprone-forward-declaration-namespace = 1.15%
  I[14:59:17.776]   bugprone-forwarding-reference-overload = 0.18%
  I[14:59:24.046]   bugprone-implicit-widening-of-multiplication-result = 6.05%
  I[14:59:29.984]   bugprone-inaccurate-erase = 0.46%
  I[14:59:35.922]   bugprone-incorrect-roundings = 0.42%
  I[14:59:42.053]   bugprone-infinite-loop = 

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

FYI I've sent D135829  to block this check 
from running in clangd, after tracking it down as the cause of a >10x 
regression in reparse times in a project that inadvertently enabled it 
(`misc-*`).

Looking at the implementation, this check seems to be expensive by design 
rather than some bug: it's matching many nodes, and then doing expensive nested 
analysis for each in the callback.

It's reasonable to want checks like that, OTOH it would be nice to have some 
way to distinguish them from the "classic" checks that aim to run in roughly 
linear time.
(It's not just clangd: we've had issues with a batch clang-tidy run over our 
codebase not finishing due to slow checks, and takes a lot of investigation 
before you can tell if this is a bug or not)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D54943#3687827 , @JonasToth wrote:

> In D54943#3681527 , @sammccall wrote:
>
>> This check is enabled by default in LLVM (`Checks: misc-*` in 
>> `llvm-project/.clang-tidy`)
>>
>> The warning on mutable non-ref local variables is pretty noisy: a *lot* of 
>> existing code does not do this, for defensible reasons (some of us think 
>> that the ratio of extra safety to extra syntactic noise for locals is too 
>> low). The LLVM style guide doesn't take a position on this.
>>
>> Should this check
>>
>> - be disabled for LLVM? (i.e. this is opt-in for codebases with strong 
>> const-correctness rules, LLVM does not, it was unintentionally enabled by 
>> `misc-*`)
>> - be configured only to warn on references? (i.e. we expect that is de-facto 
>> LLVM style and so uncontroversial)
>
> IMHO just enable for references in LLVM, thats still enough warnings to take 
> a while to process and the guide is clear on that.
> If the community wants it for values as well, that can be done later anyway.
>
> Do you think the defaults for this check should change? Originally, it was 
> "default on" because `cppcoreguidelines` state so.

I agree with Sam that this definitely needs to be disabled for LLVM -- we don't 
have anywhere near enough const correctness to get significant value out of 
this check yet, even if it was limited to just references. As for whether we 
should default to only warn on references by default -- I think that might be a 
better approach now that this is no longer tied directly to the C++ Core 
Guidelines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added a comment.

I agree with Sam on this one, especially on big files it just floods and hides 
non-stylistic diagnostics (you even hit max diagnostics exceeded errors).

I believe unless someone takes the extra mile to fix all the value types in 
LLVM to be const-correct, this is harmful then being useful. I can't speak for 
the projects out in the wild for the overall default (i have a personal 
preference for having everything const-correct, but it's mostly for stylistic 
reasons ...), but i think the default of diagnosing values for LLVM should 
definitely be off. Do you mind performing that change for `.clang-tidy` configs 
if others are not objecting on this either?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D54943#3681527 , @sammccall wrote:

> This check is enabled by default in LLVM (`Checks: misc-*` in 
> `llvm-project/.clang-tidy`)
>
> The warning on mutable non-ref local variables is pretty noisy: a *lot* of 
> existing code does not do this, for defensible reasons (some of us think that 
> the ratio of extra safety to extra syntactic noise for locals is too low). 
> The LLVM style guide doesn't take a position on this.
>
> Should this check
>
> - be disabled for LLVM? (i.e. this is opt-in for codebases with strong 
> const-correctness rules, LLVM does not, it was unintentionally enabled by 
> `misc-*`)
> - be configured only to warn on references? (i.e. we expect that is de-facto 
> LLVM style and so uncontroversial)

IMHO just enable for references in LLVM, thats still enough warnings to take a 
while to process and the guide is clear on that.
If the community wants it for values as well, that can be done later anyway.

Do you think the defaults for this check should change? Originally, it was 
"default on" because `cppcoreguidelines` state so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

This check is enabled by default in LLVM (`Checks: misc-*` in 
`llvm-project/.clang-tidy`)

The warning on mutable non-ref local variables is pretty noisy: a *lot* of 
existing code does not do this, for defensible reasons (some of us think that 
the ratio of extra safety to extra syntactic noise for locals is too low). The 
LLVM style guide doesn't take a position on this.

Should this check

- be disabled for LLVM? (i.e. this is opt-in for codebases with strong 
const-correctness rules, LLVM does not, it was unintentionally enabled by 
`misc-*`)
- be configured only to warn on references? (i.e. we expect that is de-facto 
LLVM style and so uncontroversial)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Thank you very much for all you patience, reviews and tests!
I hope that the following improvements are now simpler to integrate and that 
the test matures well. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Jonas Toth via Phabricator via cfe-commits]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit rG46ae26e7eb70: [clang-tidy] implement new check 
misc-const-correctness to add const to… (authored by 
JonasToth).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
  clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-unaligned.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-wrong-config.cpp
  clang/lib/Analysis/ExprMutationAnalyzer.cpp
  clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp

Index: clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -1251,13 +1251,13 @@
   AST =
   buildASTFromCode("void f() { int* x[2]; for (int* e : x) e = nullptr; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_TRUE(isMutated(Results, AST.get()));
 
   AST = buildASTFromCode(
   "typedef int* IntPtr;"
   "void f() { int* x[2]; for (IntPtr e : x) e = nullptr; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_TRUE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, RangeForArrayByConstRef) {
Index: clang/lib/Analysis/ExprMutationAnalyzer.cpp
===
--- clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -455,14 +455,16 @@
   // array is considered modified if the loop-variable is a non-const reference.
   const auto DeclStmtToNonRefToArray = declStmt(hasSingleDecl(varDecl(hasType(
   hasUnqualifiedDesugaredType(referenceType(pointee(arrayType(;
-  const auto RefToArrayRefToElements = match(
-  findAll(stmt(cxxForRangeStmt(
-   hasLoopVariable(varDecl(hasType(nonConstReferenceType()))
-   .bind(NodeID::value)),
-   hasRangeStmt(DeclStmtToNonRefToArray),
-   hasRangeInit(canResolveToExpr(equalsNode(Exp)
-  .bind("stmt")),
-  Stm, Context);
+  const auto RefToArrayRefToElements =
+  match(findAll(stmt(cxxForRangeStmt(
+ hasLoopVariable(
+ varDecl(anyOf(hasType(nonConstReferenceType()),
+   hasType(nonConstPointerType(
+ .bind(NodeID::value)),
+ hasRangeStmt(DeclStmtToNonRefToArray),
+ hasRangeInit(canResolveToExpr(equalsNode(Exp)
+.bind("stmt")),
+Stm, Context);
 
   if (const auto *BadRangeInitFromArray =
   selectFirst("stmt", RefToArrayRefToElements))
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-wrong-config.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-wrong-config.cpp
@@ -0,0 +1,12 @@
+// RUN: %check_clang_tidy %s misc-const-correctness %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: "misc-const-correctness.AnalyzeValues", value: false},\
+// RUN:   {key: "misc-const-correctness.AnalyzeReferences", value: false},\
+// RUN:  ]}' -- -fno-delayed-template-parsing
+
+// CHECK-MESSAGES: warning: The check 'misc-const-correctness' will not perform any analysis because both 'AnalyzeValues' and 'AnalyzeReferences' are false. [clang-tidy-config]
+
+void g() {
+  int p_local0 = 42;
+  // CHECK-FIXES-NOT: int const p_local0 = 42;
+}
Index: 

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Nathan James via Phabricator via cfe-commits]

njames93 accepted this revision.
njames93 added a comment.

LGTM with those changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

@njames93 @LegalizeAdulthood I did integrate the requested changes regarding 
warning for bad configs, refactoring of map-access and the directory structure 
of test-files and documentation.

given the high interest in the patch and the need to iron out potential 
false-positives that will only be spotted on diverse code bases, i would like 
to commit now and address outstanding issues in follow ups, that are much 
smaller and easier to manage.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 443486.
JonasToth added a comment.

- refactor: adjust warning message for mis-configuration to show which check is 
configured wrong


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
  clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-unaligned.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-wrong-config.cpp
  clang/lib/Analysis/ExprMutationAnalyzer.cpp
  clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp

Index: clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -1251,13 +1251,13 @@
   AST =
   buildASTFromCode("void f() { int* x[2]; for (int* e : x) e = nullptr; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_TRUE(isMutated(Results, AST.get()));
 
   AST = buildASTFromCode(
   "typedef int* IntPtr;"
   "void f() { int* x[2]; for (IntPtr e : x) e = nullptr; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_TRUE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, RangeForArrayByConstRef) {
Index: clang/lib/Analysis/ExprMutationAnalyzer.cpp
===
--- clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -455,14 +455,16 @@
   // array is considered modified if the loop-variable is a non-const reference.
   const auto DeclStmtToNonRefToArray = declStmt(hasSingleDecl(varDecl(hasType(
   hasUnqualifiedDesugaredType(referenceType(pointee(arrayType(;
-  const auto RefToArrayRefToElements = match(
-  findAll(stmt(cxxForRangeStmt(
-   hasLoopVariable(varDecl(hasType(nonConstReferenceType()))
-   .bind(NodeID::value)),
-   hasRangeStmt(DeclStmtToNonRefToArray),
-   hasRangeInit(canResolveToExpr(equalsNode(Exp)
-  .bind("stmt")),
-  Stm, Context);
+  const auto RefToArrayRefToElements =
+  match(findAll(stmt(cxxForRangeStmt(
+ hasLoopVariable(
+ varDecl(anyOf(hasType(nonConstReferenceType()),
+   hasType(nonConstPointerType(
+ .bind(NodeID::value)),
+ hasRangeStmt(DeclStmtToNonRefToArray),
+ hasRangeInit(canResolveToExpr(equalsNode(Exp)
+.bind("stmt")),
+Stm, Context);
 
   if (const auto *BadRangeInitFromArray =
   selectFirst("stmt", RefToArrayRefToElements))
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-wrong-config.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-wrong-config.cpp
@@ -0,0 +1,12 @@
+// RUN: %check_clang_tidy %s misc-const-correctness %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: "misc-const-correctness.AnalyzeValues", value: false},\
+// RUN:   {key: "misc-const-correctness.AnalyzeReferences", value: false},\
+// RUN:  ]}' -- -fno-delayed-template-parsing
+
+// CHECK-MESSAGES: warning: The check 'misc-const-correctness' will not perform any analysis because both 'AnalyzeValues' and 'AnalyzeReferences' are false. [clang-tidy-config]
+
+void g() {
+  int p_local0 = 42;
+  // CHECK-FIXES-NOT: int const p_local0 = 42;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
===
--- /dev/null
+++ 

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 443485.
JonasToth added a comment.

- fix: doc list and release-notes reference


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
  clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-unaligned.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-wrong-config.cpp
  clang/lib/Analysis/ExprMutationAnalyzer.cpp
  clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp

Index: clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -1251,13 +1251,13 @@
   AST =
   buildASTFromCode("void f() { int* x[2]; for (int* e : x) e = nullptr; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_TRUE(isMutated(Results, AST.get()));
 
   AST = buildASTFromCode(
   "typedef int* IntPtr;"
   "void f() { int* x[2]; for (IntPtr e : x) e = nullptr; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_TRUE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, RangeForArrayByConstRef) {
Index: clang/lib/Analysis/ExprMutationAnalyzer.cpp
===
--- clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -455,14 +455,16 @@
   // array is considered modified if the loop-variable is a non-const reference.
   const auto DeclStmtToNonRefToArray = declStmt(hasSingleDecl(varDecl(hasType(
   hasUnqualifiedDesugaredType(referenceType(pointee(arrayType(;
-  const auto RefToArrayRefToElements = match(
-  findAll(stmt(cxxForRangeStmt(
-   hasLoopVariable(varDecl(hasType(nonConstReferenceType()))
-   .bind(NodeID::value)),
-   hasRangeStmt(DeclStmtToNonRefToArray),
-   hasRangeInit(canResolveToExpr(equalsNode(Exp)
-  .bind("stmt")),
-  Stm, Context);
+  const auto RefToArrayRefToElements =
+  match(findAll(stmt(cxxForRangeStmt(
+ hasLoopVariable(
+ varDecl(anyOf(hasType(nonConstReferenceType()),
+   hasType(nonConstPointerType(
+ .bind(NodeID::value)),
+ hasRangeStmt(DeclStmtToNonRefToArray),
+ hasRangeInit(canResolveToExpr(equalsNode(Exp)
+.bind("stmt")),
+Stm, Context);
 
   if (const auto *BadRangeInitFromArray =
   selectFirst("stmt", RefToArrayRefToElements))
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-wrong-config.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-wrong-config.cpp
@@ -0,0 +1,12 @@
+// RUN: %check_clang_tidy %s misc-const-correctness %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: "misc-const-correctness.AnalyzeValues", value: false},\
+// RUN:   {key: "misc-const-correctness.AnalyzeReferences", value: false},\
+// RUN:  ]}' -- -fno-delayed-template-parsing
+
+// CHECK-MESSAGES: warning: The check will not perform any analysis because both 'AnalyzeValues' and 'AnalyzeReferences' are false. [clang-tidy-config]
+
+void g() {
+  int p_local0 = 42;
+  // CHECK-FIXES-NOT: int const p_local0 = 42;
+}
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
@@ -0,0 

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D54943#3619333 , @LegalizeAdulthood 
wrote:

> Clang-tidy tests and docs have been moved to subdirectories by module, please 
> rebase to `main:HEAD`

I moved the tests and the documentation as well.
tests + documentation do build/pass


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/misc-const-correctness.rst:10
+`CppCoreGuidelines ES.25 
`_
+and `AUTOSAR C++14 Rule A7-1-1 (6.7.1 Specifiers) 
`_.
+

carlosgalvezp wrote:
> AUTOSAR mentions should be removed as per previous comment from 
> @aaron.ballman 
Why? I dont think its wrong to hint that there are coding standards that 
require `const` to be used consistently.
The check does not run under `cppcoreguidelines-` because the guidelines are 
not exact on their requirements. But "the spirit" is still the same. I would 
like to keep the references.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 443484.
JonasToth marked 4 inline comments as done.
JonasToth added a comment.
Herald added a reviewer: jdoerfert.
Herald added subscribers: abrachet, sstefan1, phosek.

- test: move tests into group specific directory
- refactor: use more idiomatic c++ for scope-based analyzer creation/access
- feat: provide config validation if analysis if completely configured off
- refactor: move documentation as well
- test: add test for incorrect configurations


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
  clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-unaligned.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-wrong-config.cpp
  clang/lib/Analysis/ExprMutationAnalyzer.cpp
  clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp

Index: clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -1251,13 +1251,13 @@
   AST =
   buildASTFromCode("void f() { int* x[2]; for (int* e : x) e = nullptr; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_TRUE(isMutated(Results, AST.get()));
 
   AST = buildASTFromCode(
   "typedef int* IntPtr;"
   "void f() { int* x[2]; for (IntPtr e : x) e = nullptr; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_TRUE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, RangeForArrayByConstRef) {
Index: clang/lib/Analysis/ExprMutationAnalyzer.cpp
===
--- clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -455,14 +455,16 @@
   // array is considered modified if the loop-variable is a non-const reference.
   const auto DeclStmtToNonRefToArray = declStmt(hasSingleDecl(varDecl(hasType(
   hasUnqualifiedDesugaredType(referenceType(pointee(arrayType(;
-  const auto RefToArrayRefToElements = match(
-  findAll(stmt(cxxForRangeStmt(
-   hasLoopVariable(varDecl(hasType(nonConstReferenceType()))
-   .bind(NodeID::value)),
-   hasRangeStmt(DeclStmtToNonRefToArray),
-   hasRangeInit(canResolveToExpr(equalsNode(Exp)
-  .bind("stmt")),
-  Stm, Context);
+  const auto RefToArrayRefToElements =
+  match(findAll(stmt(cxxForRangeStmt(
+ hasLoopVariable(
+ varDecl(anyOf(hasType(nonConstReferenceType()),
+   hasType(nonConstPointerType(
+ .bind(NodeID::value)),
+ hasRangeStmt(DeclStmtToNonRefToArray),
+ hasRangeInit(canResolveToExpr(equalsNode(Exp)
+.bind("stmt")),
+Stm, Context);
 
   if (const auto *BadRangeInitFromArray =
   selectFirst("stmt", RefToArrayRefToElements))
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-wrong-config.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-wrong-config.cpp
@@ -0,0 +1,12 @@
+// RUN: %check_clang_tidy %s misc-const-correctness %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: "misc-const-correctness.AnalyzeValues", value: false},\
+// RUN:   {key: "misc-const-correctness.AnalyzeReferences", value: false},\
+// RUN:  ]}' -- -fno-delayed-template-parsing
+
+// CHECK-MESSAGES: warning: The check will not perform any analysis because both 'AnalyzeValues' and 'AnalyzeReferences' 

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Carlos Galvez via Phabricator via cfe-commits]

carlosgalvezp added inline comments.
Herald added a reviewer: NoQ.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/misc-const-correctness.rst:10
+`CppCoreGuidelines ES.25 
`_
+and `AUTOSAR C++14 Rule A7-1-1 (6.7.1 Specifiers) 
`_.
+

AUTOSAR mentions should be removed as per previous comment from @aaron.ballman 


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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Richard via Phabricator via cfe-commits]

LegalizeAdulthood requested changes to this revision.
LegalizeAdulthood added a comment.
This revision now requires changes to proceed.

Clang-tidy tests and docs have been moved to subdirectories by module, please 
rebase to `main:HEAD`


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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h:35
+TransformPointersAsValues(
+Options.get("TransformPointersAsValues", false)) {}
+

JonasToth wrote:
> njames93 wrote:
> > It may be worth adding some validation to these. If AnalyzeValues, 
> > AnalyzeReferences and WarnPointersAsValues are all false, this whole check 
> > is basically a no-op.
> Whats the proper way to react? Short-circuit the `registerMatchers`, similar 
> to language support?
> I think thats the way I would go about this.
`ClangTidyCheck::configurationDiag` is the way to warn about issues with config.


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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h:35
+TransformPointersAsValues(
+Options.get("TransformPointersAsValues", false)) {}
+

njames93 wrote:
> It may be worth adding some validation to these. If AnalyzeValues, 
> AnalyzeReferences and WarnPointersAsValues are all false, this whole check is 
> basically a no-op.
Whats the proper way to react? Short-circuit the `registerMatchers`, similar to 
language support?
I think thats the way I would go about this.


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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Nathan James via Phabricator via cfe-commits]

njames93 accepted this revision.
njames93 added a comment.
This revision is now accepted and ready to land.

LGTM, just a couple points but not essential.




Comment at: clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp:184-187
+  if (ScopesCache.find(LocalScope) == ScopesCache.end())
+ScopesCache.insert(std::make_pair(
+LocalScope,
+std::make_unique(*LocalScope, *Context)));





Comment at: clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h:35
+TransformPointersAsValues(
+Options.get("TransformPointersAsValues", false)) {}
+

It may be worth adding some validation to these. If AnalyzeValues, 
AnalyzeReferences and WarnPointersAsValues are all false, this whole check is 
basically a no-op.


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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

ping :)
@njames93  I added more `CHECK-FIXES` and `CHECK-FIXES-NOT` statements in the 
tests.


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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 432774.
JonasToth added a comment.

- addded `CHECK-FIXES` in clang-tidy tests
- merged latest main into branch


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

https://reviews.llvm.org/D54943

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc-const-correctness.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-unaligned.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-values.cpp
  clang/lib/Analysis/ExprMutationAnalyzer.cpp
  clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp

Index: clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -1251,13 +1251,13 @@
   AST =
   buildASTFromCode("void f() { int* x[2]; for (int* e : x) e = nullptr; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_TRUE(isMutated(Results, AST.get()));
 
   AST = buildASTFromCode(
   "typedef int* IntPtr;"
   "void f() { int* x[2]; for (IntPtr e : x) e = nullptr; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_TRUE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, RangeForArrayByConstRef) {
Index: clang/lib/Analysis/ExprMutationAnalyzer.cpp
===
--- clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -455,14 +455,16 @@
   // array is considered modified if the loop-variable is a non-const reference.
   const auto DeclStmtToNonRefToArray = declStmt(hasSingleDecl(varDecl(hasType(
   hasUnqualifiedDesugaredType(referenceType(pointee(arrayType(;
-  const auto RefToArrayRefToElements = match(
-  findAll(stmt(cxxForRangeStmt(
-   hasLoopVariable(varDecl(hasType(nonConstReferenceType()))
-   .bind(NodeID::value)),
-   hasRangeStmt(DeclStmtToNonRefToArray),
-   hasRangeInit(canResolveToExpr(equalsNode(Exp)
-  .bind("stmt")),
-  Stm, Context);
+  const auto RefToArrayRefToElements =
+  match(findAll(stmt(cxxForRangeStmt(
+ hasLoopVariable(
+ varDecl(anyOf(hasType(nonConstReferenceType()),
+   hasType(nonConstPointerType(
+ .bind(NodeID::value)),
+ hasRangeStmt(DeclStmtToNonRefToArray),
+ hasRangeInit(canResolveToExpr(equalsNode(Exp)
+.bind("stmt")),
+Stm, Context);
 
   if (const auto *BadRangeInitFromArray =
   selectFirst("stmt", RefToArrayRefToElements))
Index: clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-values.cpp
@@ -0,0 +1,1030 @@
+// RUN: %check_clang_tidy %s misc-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'misc-const-correctness.TransformValues', value: true}, \
+// RUN:   {key: 'misc-const-correctness.WarnPointersAsValues', value: false}, \
+// RUN:   {key: 'misc-const-correctness.TransformPointersAsValues', value: false}, \
+// RUN:   ]}" -- -fno-delayed-template-parsing
+
+// --- Provide test samples for primitive builtins -
+// - every 'p_*' variable is a 'potential_const_*' variable
+// - every 'np_*' variable is a 'non_potential_const_*' variable
+
+bool global;
+char np_global = 0; // globals can't be known to be const
+
+// FIXME: 'static' globals are not matched right now. They could be analyzed but aren't right now.
+static int p_static_global = 42;
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} 

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

Pretty much good to go, just a few nits with the tests.
Can you add CHECK-FIXES directives for all warnings if there should be a fixit.
If there shouldn't be one could you either add a comment saying there shouldn't 
be one, or put a CHECK-FIXES-NOT directive.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

friendly ping :) (maybe @njames93 ?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 427629.
JonasToth added a comment.

- fix documentation reference


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc-const-correctness.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-unaligned.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-values.cpp
  clang/lib/Analysis/ExprMutationAnalyzer.cpp
  clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp

Index: clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -1251,13 +1251,13 @@
   AST =
   buildASTFromCode("void f() { int* x[2]; for (int* e : x) e = nullptr; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_TRUE(isMutated(Results, AST.get()));
 
   AST = buildASTFromCode(
   "typedef int* IntPtr;"
   "void f() { int* x[2]; for (IntPtr e : x) e = nullptr; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_TRUE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, RangeForArrayByConstRef) {
Index: clang/lib/Analysis/ExprMutationAnalyzer.cpp
===
--- clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -445,14 +445,16 @@
   // array is considered modified if the loop-variable is a non-const reference.
   const auto DeclStmtToNonRefToArray = declStmt(hasSingleDecl(varDecl(hasType(
   hasUnqualifiedDesugaredType(referenceType(pointee(arrayType(;
-  const auto RefToArrayRefToElements = match(
-  findAll(stmt(cxxForRangeStmt(
-   hasLoopVariable(varDecl(hasType(nonConstReferenceType()))
-   .bind(NodeID::value)),
-   hasRangeStmt(DeclStmtToNonRefToArray),
-   hasRangeInit(canResolveToExpr(equalsNode(Exp)
-  .bind("stmt")),
-  Stm, Context);
+  const auto RefToArrayRefToElements =
+  match(findAll(stmt(cxxForRangeStmt(
+ hasLoopVariable(
+ varDecl(anyOf(hasType(nonConstReferenceType()),
+   hasType(nonConstPointerType(
+ .bind(NodeID::value)),
+ hasRangeStmt(DeclStmtToNonRefToArray),
+ hasRangeInit(canResolveToExpr(equalsNode(Exp)
+.bind("stmt")),
+Stm, Context);
 
   if (const auto *BadRangeInitFromArray =
   selectFirst("stmt", RefToArrayRefToElements))
Index: clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-values.cpp
@@ -0,0 +1,958 @@
+// RUN: %check_clang_tidy %s misc-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'misc-const-correctness.TransformValues', value: true}, \
+// RUN:   {key: 'misc-const-correctness.WarnPointersAsValues', value: false}, \
+// RUN:   {key: 'misc-const-correctness.TransformPointersAsValues', value: false}, \
+// RUN:   ]}" -- -fno-delayed-template-parsing
+
+// --- Provide test samples for primitive builtins -
+// - every 'p_*' variable is a 'potential_const_*' variable
+// - every 'np_*' variable is a 'non_potential_const_*' variable
+
+bool global;
+char np_global = 0; // globals can't be known to be const
+
+// FIXME: 'static' globals are not matched right now. They could be analyzed but aren't right now.
+static int p_static_global = 42;
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // 

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h:24
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-const.html
+class ConstCorrectnessCheck : public ClangTidyCheck {

missing change to new docs


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Jonas Toth via Phabricator via cfe-commits]

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

Thank you for the review! I adjusted the patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 427423.
JonasToth marked 3 inline comments as done.
JonasToth added a comment.

- addressing review comments and remove unrelated changes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

Files:
  clang-tools-extra/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  clang-tools-extra/docs/clang-tidy/checks/misc-const-correctness.rst
  clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-unaligned.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-values.cpp
  clang/lib/Analysis/ExprMutationAnalyzer.cpp
  clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp

Index: clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
===
--- clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
+++ clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp
@@ -1251,13 +1251,13 @@
   AST =
   buildASTFromCode("void f() { int* x[2]; for (int* e : x) e = nullptr; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_TRUE(isMutated(Results, AST.get()));
 
   AST = buildASTFromCode(
   "typedef int* IntPtr;"
   "void f() { int* x[2]; for (IntPtr e : x) e = nullptr; }");
   Results = match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
-  EXPECT_FALSE(isMutated(Results, AST.get()));
+  EXPECT_TRUE(isMutated(Results, AST.get()));
 }
 
 TEST(ExprMutationAnalyzerTest, RangeForArrayByConstRef) {
Index: clang/lib/Analysis/ExprMutationAnalyzer.cpp
===
--- clang/lib/Analysis/ExprMutationAnalyzer.cpp
+++ clang/lib/Analysis/ExprMutationAnalyzer.cpp
@@ -445,14 +445,16 @@
   // array is considered modified if the loop-variable is a non-const reference.
   const auto DeclStmtToNonRefToArray = declStmt(hasSingleDecl(varDecl(hasType(
   hasUnqualifiedDesugaredType(referenceType(pointee(arrayType(;
-  const auto RefToArrayRefToElements = match(
-  findAll(stmt(cxxForRangeStmt(
-   hasLoopVariable(varDecl(hasType(nonConstReferenceType()))
-   .bind(NodeID::value)),
-   hasRangeStmt(DeclStmtToNonRefToArray),
-   hasRangeInit(canResolveToExpr(equalsNode(Exp)
-  .bind("stmt")),
-  Stm, Context);
+  const auto RefToArrayRefToElements =
+  match(findAll(stmt(cxxForRangeStmt(
+ hasLoopVariable(
+ varDecl(anyOf(hasType(nonConstReferenceType()),
+   hasType(nonConstPointerType(
+ .bind(NodeID::value)),
+ hasRangeStmt(DeclStmtToNonRefToArray),
+ hasRangeInit(canResolveToExpr(equalsNode(Exp)
+.bind("stmt")),
+Stm, Context);
 
   if (const auto *BadRangeInitFromArray =
   selectFirst("stmt", RefToArrayRefToElements))
Index: clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-values.cpp
@@ -0,0 +1,958 @@
+// RUN: %check_clang_tidy %s misc-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'misc-const-correctness.TransformValues', value: true}, \
+// RUN:   {key: 'misc-const-correctness.WarnPointersAsValues', value: false}, \
+// RUN:   {key: 'misc-const-correctness.TransformPointersAsValues', value: false}, \
+// RUN:   ]}" -- -fno-delayed-template-parsing
+
+// --- Provide test samples for primitive builtins -
+// - every 'p_*' variable is a 'potential_const_*' variable
+// - every 'np_*' variable is a 'non_potential_const_*' variable
+
+bool global;
+char np_global = 0; // globals can't be known to be const
+
+// FIXME: 'static' globals are not matched right now. They could be analyzed but aren't right now.
+static int p_static_global = 42;
+
+namespace foo {
+int scoped;

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp:103
+
+#if 0
+  // FIXME: Remove this section if there are no crashes after the iterator-fix.

Hasn't this already been addressed, if so can this block just be removed.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:103
 - New :doc:`bugprone-shared-ptr-array-mismatch 
` check.
-
   Finds initializations of C++ shared pointers to non-array type that are 
initialized with an array.

Unrelated change.



Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:87
`bugprone-reserved-identifier `_, "Yes"
-   `bugprone-shared-ptr-array-mismatch 
`_, "Yes"
`bugprone-signal-handler `_,

This should be committed separately.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc-const-correctness-transform-values.cpp:27
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' 
can be declared 'const'
+  // CHECK-FIXES: const
+}

This check directive isn't going to be effective.
Try `CHECK-FIXES: const int p_local0 = 2;`
Same goes for all the ones below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54943

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits