[PATCH] D130793: [clang-tidy] adjust treating of array-of-pointers when 'AnalyzePointers' is deactivated

[Jonas Toth via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGe66345d54d5f: [clang-tidy] adjust treating of 
array-of-pointers when AnalyzePointers is… (authored by JonasToth).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130793

Files:
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.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-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
@@ -526,18 +526,13 @@
   // CHECK-FIXES: int const p_local1[2]
   for (const int _ref : p_local1) {
   }
+}
 
-  int *p_local2[2] = {_local0[0], _local0[1]};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local2[2]
-  for (const int *con_ptr : p_local2) {
-  }
-
-  int *p_local3[2] = {nullptr, nullptr};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'int *[2]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local3[2]
-  for (const auto *con_ptr : p_local3) {
-  }
+void arrays_of_pointers_are_ignored() {
+  int *np_local0[2] = {nullptr, nullptr};
+  
+  using intPtr = int*;
+  intPtr np_local1[2] = {nullptr, nullptr};
 }
 
 inline void *operator new(decltype(sizeof(void *)), void *p) { return p; }
@@ -908,41 +903,6 @@
   sizeof(int[++N]);
 }
 
-template 
-struct SmallVectorBase {
-  T data[4];
-  void push_back(const T ) {}
-  int size() const { return 4; }
-  T *begin() { return data; }
-  const T *begin() const { return data; }
-  T *end() { return data + 4; }
-  const T *end() const { return data + 4; }
-};
-
-template 
-struct SmallVector : SmallVectorBase {};
-
-template 
-void EmitProtocolMethodList(T &) {
-  // Note: If the template is uninstantiated the analysis does not figure out,
-  // that p_local0 could be const. Not sure why, but probably bails because
-  // some expressions are type-dependent.
-  SmallVector p_local0;
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'SmallVector' can be declared 'const'
-  // CHECK-FIXES: SmallVector const p_local0
-  SmallVector np_local0;
-  for (const auto *I : Methods) {
-if (I == nullptr)
-  np_local0.push_back(I);
-  }
-  p_local0.size();
-}
-void instantiate() {
-  int *p_local0[4] = {nullptr, nullptr, nullptr, nullptr};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[4]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local0[4]
-  EmitProtocolMethodList(p_local0);
-}
 struct base {
   int member;
 };
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.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-pointer-as-values.cpp
@@ -10,4 +10,65 @@
   double *p_local0 = _local0[1];
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double *' can be declared 'const'
   // CHECK-FIXES: double *const p_local0
+
+  using doublePtr = double*;
+  using doubleArray = double[15];
+  doubleArray np_local1;
+  doublePtr p_local1 = _local1[0];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'doublePtr' (aka 'double *') can be declared 'const'
+  // CHECK-FIXES: doublePtr const p_local1
+}
+
+void range_for() {
+  int np_local0[2] = {1, 2};
+  int *p_local0[2] = {_local0[0], _local0[1]};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[2]' can be declared 'const'
+  // CHECK-FIXES: int *const p_local0[2]
+  for (const int *p_local1 : p_local0) {
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'p_local1' of type 'const int *' can be declared 'const'
+  // CHECK-FIXES: for (const int *const p_local1 : p_local0)
+  }
+
+  int *p_local2[2] = {nullptr, nullptr};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
+  // CHECK-FIXES: int *const p_local2[2]
+  for (const auto *con_ptr : p_local2) {
+  }
+
+}
+
+template 
+struct SmallVectorBase {
+  T data[4];
+  void push_back(const T ) {}
+  int size() const { return 4; }
+  T *begin() { return data; }
+  const T *begin() const { return data; }
+  T *end() { return data + 4; }
+  const T *end() const { return data + 4; }
+};
+
+template 
+struct SmallVector : SmallVectorBase {};
+
+template 
+void EmitProtocolMethodList(T &) 

[PATCH] D130793: [clang-tidy] adjust treating of array-of-pointers when 'AnalyzePointers' is deactivated

[Jonas Toth via Phabricator via cfe-commits]

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

- remove unproductive check-fixes line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130793

Files:
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.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-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
@@ -526,18 +526,13 @@
   // CHECK-FIXES: int const p_local1[2]
   for (const int _ref : p_local1) {
   }
+}
 
-  int *p_local2[2] = {_local0[0], _local0[1]};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local2[2]
-  for (const int *con_ptr : p_local2) {
-  }
-
-  int *p_local3[2] = {nullptr, nullptr};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'int *[2]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local3[2]
-  for (const auto *con_ptr : p_local3) {
-  }
+void arrays_of_pointers_are_ignored() {
+  int *np_local0[2] = {nullptr, nullptr};
+  
+  using intPtr = int*;
+  intPtr np_local1[2] = {nullptr, nullptr};
 }
 
 inline void *operator new(decltype(sizeof(void *)), void *p) { return p; }
@@ -908,41 +903,6 @@
   sizeof(int[++N]);
 }
 
-template 
-struct SmallVectorBase {
-  T data[4];
-  void push_back(const T ) {}
-  int size() const { return 4; }
-  T *begin() { return data; }
-  const T *begin() const { return data; }
-  T *end() { return data + 4; }
-  const T *end() const { return data + 4; }
-};
-
-template 
-struct SmallVector : SmallVectorBase {};
-
-template 
-void EmitProtocolMethodList(T &) {
-  // Note: If the template is uninstantiated the analysis does not figure out,
-  // that p_local0 could be const. Not sure why, but probably bails because
-  // some expressions are type-dependent.
-  SmallVector p_local0;
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'SmallVector' can be declared 'const'
-  // CHECK-FIXES: SmallVector const p_local0
-  SmallVector np_local0;
-  for (const auto *I : Methods) {
-if (I == nullptr)
-  np_local0.push_back(I);
-  }
-  p_local0.size();
-}
-void instantiate() {
-  int *p_local0[4] = {nullptr, nullptr, nullptr, nullptr};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[4]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local0[4]
-  EmitProtocolMethodList(p_local0);
-}
 struct base {
   int member;
 };
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.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-pointer-as-values.cpp
@@ -10,4 +10,65 @@
   double *p_local0 = _local0[1];
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double *' can be declared 'const'
   // CHECK-FIXES: double *const p_local0
+
+  using doublePtr = double*;
+  using doubleArray = double[15];
+  doubleArray np_local1;
+  doublePtr p_local1 = _local1[0];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'doublePtr' (aka 'double *') can be declared 'const'
+  // CHECK-FIXES: doublePtr const p_local1
+}
+
+void range_for() {
+  int np_local0[2] = {1, 2};
+  int *p_local0[2] = {_local0[0], _local0[1]};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[2]' can be declared 'const'
+  // CHECK-FIXES: int *const p_local0[2]
+  for (const int *p_local1 : p_local0) {
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'p_local1' of type 'const int *' can be declared 'const'
+  // CHECK-FIXES: for (const int *const p_local1 : p_local0)
+  }
+
+  int *p_local2[2] = {nullptr, nullptr};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
+  // CHECK-FIXES: int *const p_local2[2]
+  for (const auto *con_ptr : p_local2) {
+  }
+
+}
+
+template 
+struct SmallVectorBase {
+  T data[4];
+  void push_back(const T ) {}
+  int size() const { return 4; }
+  T *begin() { return data; }
+  const T *begin() const { return data; }
+  T *end() { return data + 4; }
+  const T *end() const { return data + 4; }
+};
+
+template 
+struct SmallVector : SmallVectorBase {};
+
+template 
+void EmitProtocolMethodList(T &) {
+  // Note: If the template is uninstantiated the analysis does not figure out,
+  // that p_local0 could be const. Not sure why, but probably 

[PATCH] D130181: [clang-tidy] Add readability-use-early-exits check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D130181#3769618 , @njames93 wrote:

> In D130181#3769083 , @JonasToth 
> wrote:
>
>> ...
>
> Your concerns aren't actually that important. Because the transformations 
> only work on for binary operators, and not CXXOperatorCallExpr, it would 
> always never do any special logic, instead just wrap the whole thing in 
> parens and negate it.

Ah yes, you are right! That makes it very much better.

> I think the best way to resolve that could be delayed diagnostics for 
> template operators.
> So cache the locations of all dependent binary (and unary) operators, and see 
> if any of them either stay as binary operators or get transformed into 
> CXXOperatorCallExprs, if so don't emit the fix.
> The second option is to just not emit any fix for template dependent 
> operators.
> WDYT?

I am not 100% sure that delayed diagnostics would solve the issue _always_

  template 
  bool type_dependent_logic(T1 foo, T2 bar) {
  return foo && !bar;
  }

This template would be a counter example to yours, where I feel less 
comfortable.
The definition itself has `BinaryOperator` in the AST.

Now different TUs might use this template wildly different, e.g. TU1.cpp only 
with builtins, making a transformation possible and TU2.cpp with fancy and 
weird types making the transformation not possible.

clang-tidy (e.g. with `run-clang-tidy`) would now still change the template 
definition after it received the diagnostic from TU1.cpp, even though in 
TU2.cpp it is not considered as valid.
This issue is common to many checks, e.g. the `const-correctness` suffered from 
it as well. In the `const` case I had to just had to consider type dependent 
expressions as modifications.

For TU-local templates your strategy is certainly viable. But in the general 
case I would rather not do it, or at least have a "default-off" option for it.

Finally, what are your thoughts on `optional` types? They are kinda `bool` in 
the sense of this check, but suffer from the shortcomings with overloaded 
operators.
Should they be explicitly not considered now (i am fine with that!)? In that 
case, it should be mentioned as limitation in the documentation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130181

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

[PATCH] D130793: [clang-tidy] adjust treating of array-of-pointers when 'AnalyzePointers' is deactivated

[Jonas Toth via Phabricator via cfe-commits]

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

ping @njames93 :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130793

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

[PATCH] D130181: [clang-tidy] Add readability-use-early-exits check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

IMHO the check looks good, but I do have concerns about correctness of the 
transformation in specific cases, especially overloaded operators.

If the conditions are inverted, it might be the case that the inverted operator 
is not overloaded, resulting in compilation errors.
I think that should be guarded against.

And then there is the more subtle issue of different semantics of the operators.

Hypothetical Matrix Algebra Situation:

  // A, B are matrices of booleans with identical dimensions
  // A && B will do '&&' on each matrix element
  // The matrices overload 'operator bool()' for implicit conversion to 'bool',
  // true := any element != false
  // false := all false
  if (A && B) {
// !C inverts each boolean in C, same '&&' application
if (B && !C) {
  padLines();
}
  }

Transformed to

  if (!A )
continue;
  if ( !B)
continue;
  if (!B )
continue;
  if ( C)
continue;
  padLines();

is highly likely not a correct and equivalent transformation.

My point is not so much about the concrete example, but more general.
C++ allows to implement operators with different semantics and classes must not 
behave like 'int' for the operators. Such overloads are not necessarily 
incorrect. E.g. `boost::sml` uses the operator overloading to define state 
machines, which does not follow anything close to the semantics we need for 
this check.
Expression-template libraries (especially linear algebra and so on) might 
either break or change meaning as well.

In my personal opinion the check should at least have an option to disable 
transformations for classes with overloaded operators and verify that the 
transformation would still compile if done anyway.
I think even better would be a "concept check" to at least verify that the type 
in question models a boolean with its operators. But I am not sure how far such 
a check should go and I am aware that it would not be perfect anyway.




Comment at: clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp:329
+assert(BSize >= CheckAlias.size() + OptName.size());
+memcpy(Buff, CheckAlias.data(), CheckAlias.size());
+memcpy(Buff + CheckAlias.size(), OptName.data(), OptName.size());

i would really prefer to just use strings here.
`std::string Buff = CheckAlias.str() + OptName.str();` is much easier to 
understand and equivalent (?)
 
this function is called only once in the constructor as well, so speed and 
allocations are not valid in my opinion.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst:63
+void Process(bool A, bool B) {
+  if (A && B) {
+// Long processing.

njames93 wrote:
> JonasToth wrote:
> > njames93 wrote:
> > > JonasToth wrote:
> > > > if this option is false, the transformation would be `if(!(A && B))`, 
> > > > right?
> > > > 
> > > > should demorgan rules be applied or at least be mentioned here? I think 
> > > > transforming to `if (!A || !B)` is at least a viable option for enough 
> > > > users.
> > > Once this is in, I plan to merge some common code with the 
> > > simplify-boolean-expr logic for things like demorgan processing. Right 
> > > now the transformation happens, the simplify boolean suggests a demorgan 
> > > transformation of you run the output through clang tidy.
> > a short reference to the `readability-simplify-boolean-expr` check in the 
> > user facing docs would be great.
> > i personally find it ok if the users "pipe" multiple checks together to 
> > reach a final transformation.
> > 
> > would this check then use the same settings as the 
> > `readability-simplify-boolean-expr` check? (that of course off topic and 
> > does not relate to this patch :) )
> I'm not sure it's really necessary to mention that the fix would likely need 
> another fix, besides that comment would just be removed in the follow up.
ok, thats good with me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130181

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

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-alignment.cpp:43
+  // of the `*`.
+  int *IPtr = 
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'IPtr' of type 'int *' 
can be declared 'const'

could you please provide more test cases with pointers in combination with

- macros
- typedefs
- template parameters
- `auto *`

I think we should be thorough here. The users might not have all details of 
where `const` applies in their forehead and big transformations of code-bases 
should definitely not break the code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131386

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

[PATCH] D131386: [clang-tidy] Added `ConstAlignment` option to `misc-const-correctness`

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

during the original implementation of the fix-util I had the plan to provide an 
option for east-vs-west const, but during the discussions we came to the 
conclusion that `clang-format` should do this (the patches were already pending 
at that point in time).

if this option works, i think its ok to provide it. having this check on during 
development is maybe just annoying when the `const` always pops up at the 
"wrong" side.

but i think we should provide more test cases here, especially for the edge 
cases where `const` _must_ be on the right side, because it would otherwise 
apply to something else then intended. that was the original reason why i 
sticked to east-const, because thats always correct.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131386

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

[PATCH] D130181: [clang-tidy] Add readability-use-early-exits check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst:63
+void Process(bool A, bool B) {
+  if (A && B) {
+// Long processing.

njames93 wrote:
> JonasToth wrote:
> > if this option is false, the transformation would be `if(!(A && B))`, right?
> > 
> > should demorgan rules be applied or at least be mentioned here? I think 
> > transforming to `if (!A || !B)` is at least a viable option for enough 
> > users.
> Once this is in, I plan to merge some common code with the 
> simplify-boolean-expr logic for things like demorgan processing. Right now 
> the transformation happens, the simplify boolean suggests a demorgan 
> transformation of you run the output through clang tidy.
a short reference to the `readability-simplify-boolean-expr` check in the user 
facing docs would be great.
i personally find it ok if the users "pipe" multiple checks together to reach a 
final transformation.

would this check then use the same settings as the 
`readability-simplify-boolean-expr` check? (that of course off topic and does 
not relate to this patch :) )


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130181

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

[PATCH] D130181: [clang-tidy] Add readability-use-early-exits check

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

just my 2 cents




Comment at: clang-tools-extra/clang-tidy/readability/UseEarlyExitsCheck.cpp:66
+  if (needsParensAfterUnaryNegation(Condition)) {
+Diag << FixItHint::CreateInsertion(Condition->getBeginLoc(), "!(")
+ << FixItHint::CreateInsertion(

did you consider comma expressions?

`if (myDirtyCode(), myCondition && yourCondition)`. It seems to me, that the 
transformation would be incorrect.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst:55
+
+  If `true`, split up conditions with cunjunctions (``&&``) into multiple 
+  ``if`` statements. Default value is `false`.

typo `cunjunctions -> conjunctions`



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst:63
+void Process(bool A, bool B) {
+  if (A && B) {
+// Long processing.

if this option is false, the transformation would be `if(!(A && B))`, right?

should demorgan rules be applied or at least be mentioned here? I think 
transforming to `if (!A || !B)` is at least a viable option for enough users.



Comment at: 
clang-tools-extra/docs/clang-tidy/checks/readability/use-early-exits.rst:112
+
+  If `true`, the check will only report ifs which contain nested control 
blocks.
+  Default value is `false`

maybe highlight the `if` as code with double quotes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130181

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

[PATCH] D132244: [docs] improve documentation for misc-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGb5b750346346: [docs] improve documentation for 
misc-const-correctness (authored by JonasToth).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132244

Files:
  clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst

Index: clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
@@ -4,12 +4,12 @@
 ==
 
 This check implements detection of local variables which could be declared as
-``const``, but are not. Declaring variables as ``const`` is required or recommended by many
+``const`` but are not. Declaring variables as ``const`` is required or recommended by many
 coding guidelines, such as:
 `CppCoreGuidelines ES.25 `_
 and `AUTOSAR C++14 Rule A7-1-1 (6.7.1 Specifiers) `_.
 
-Please note that this analysis is type-based only. Variables that are not modified
+Please note that this check's analysis is type-based only. Variables that are not modified
 but used to create a non-const handle that might escape the scope are not diagnosed
 as potential ``const``.
 
@@ -18,25 +18,29 @@
   // Declare a variable, which is not ``const`` ...
   int i = 42;
   // but use it as read-only. This means that `i` can be declared ``const``.
-  int result = i * i;
+  int result = i * i;   // Before transformation
+  int const result = i * i; // After transformation
 
-The check can analyzes values, pointers and references but not (yet) pointees:
+The check can analyze values, pointers and references but not (yet) pointees:
 
 .. code-block:: c++
 
   // Normal values like built-ins or objects.
-  int potential_const_int = 42; // 'const int potential_const_int = 42' suggestion.
+  int potential_const_int = 42;   // Before transformation
+  int const potential_const_int = 42; // After transformation
   int copy_of_value = potential_const_int;
 
-  MyClass could_be_const; // 'const MyClass could_be_const' suggestion;
+  MyClass could_be_const;   // Before transformation
+  MyClass const could_be_const; // After transformation
   could_be_const.const_qualified_method();
 
   // References can be declared const as well.
-  int _value = potential_const_int; // 'const int _value' suggestion.
+  int _value = potential_const_int;   // Before transformation
+  int const& reference_value = potential_const_int; // After transformation
   int another_copy = reference_value;
 
   // The similar semantics of pointers are not (yet) analyzed.
-  int *pointer_variable = _const_int; // Not 'const int *pointer_variable' suggestion.
+  int *pointer_variable = _const_int; // _NO_ 'const int *pointer_variable' suggestion.
   int last_copy = *pointer_variable;
 
 The automatic code transformation is only applied to variables that are declared in single
@@ -44,18 +48,20 @@
 `readability-isolate-declaration <../readability/isolate-declaration.html>`_ first.
 
 Note that there is the check
-`cppcoreguidelines-avoid-non-const-global-variables `_
+`cppcoreguidelines-avoid-non-const-global-variables <../cppcoreguidelines/avoid-non-const-global-variables.html>`_
 to enforce ``const`` correctness on all globals.
 
 Known Limitations
 -
 
+The check does not run on `C` code.
+
 The check will not analyze templated variables or variables that are instantiation dependent.
 Different instantiations can result in different ``const`` correctness properties and in general it
-is not possible to find all instantiations of a template. It might be used differently in an
-independent translation unit.
+is not possible to find all instantiations of a template. The template might be used differently in
+an independent translation unit.
 
-Pointees can not be analyzed for constness yet. The following code is shows this limitation.
+Pointees can not be analyzed for constness yet. The following code shows this limitation.
 
 .. code-block:: c++
 
@@ -74,15 +80,35 @@
 Options
 ---
 
-.. option:: AnalyzeValues (default = 1)
+.. option:: AnalyzeValues (default = true)
 
   Enable or disable the analysis of ordinary value variables, like ``int i = 42;``
 
-.. option:: AnalyzeReferences (default = 1)
+  .. code-block:: c++
+
+// Warning
+int i = 42;
+// No warning
+int const i = 42;
+
+// Warning
+int a[] = {42, 42, 42};
+// No warning
+int const a[] = {42, 42, 42};
+
+.. option:: AnalyzeReferences (default = true)
 
   

[PATCH] D130793: [clang-tidy] adjust treating of array-of-pointers when 'AnalyzePointers' is deactivated

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.cpp:27-30
+  for (const int *p_local1 : p_local0) {
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'p_local1' of type 
'const int *' can be declared 'const'
+  // CHECK-FIXES: for (const int *const p_local1 : p_local0)
+  }

njames93 wrote:
> Wouldn't this behaviour be expected before the changes added in this patch, 
> as p_local1 isn't an array type.
the behaviour for `p_local1` does not change with the patch and is tested/moved 
here.

only `p_local0` has a behaviour change. this test file activates the analysis 
for `pointers-as-values`, so it must be diagnosed in this test file.
in the other test-file `-values.cpp` `p_local0` is not diagnosed, making it 
`np_local0`.

originally, the tests for the range based for loops lived in the `values` part 
of the tests. As this is incorrect for the pointer-arrays I moved them into 
this file instead (for pointer arrays).
the tests here mostly verify that the variables are properly matched and 
filtered. The actual analysis has its own unit tests for these cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130793

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

[PATCH] D132244: [docs] improve documentation for misc-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

@njames93 friendly ping, for https://reviews.llvm.org/D130793 as well :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132244

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

[PATCH] D132244: [docs] improve documentation for misc-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- mention C limitation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132244

Files:
  clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst

Index: clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
@@ -4,12 +4,12 @@
 ==
 
 This check implements detection of local variables which could be declared as
-``const``, but are not. Declaring variables as ``const`` is required or recommended by many
+``const`` but are not. Declaring variables as ``const`` is required or recommended by many
 coding guidelines, such as:
 `CppCoreGuidelines ES.25 `_
 and `AUTOSAR C++14 Rule A7-1-1 (6.7.1 Specifiers) `_.
 
-Please note that this analysis is type-based only. Variables that are not modified
+Please note that this check's analysis is type-based only. Variables that are not modified
 but used to create a non-const handle that might escape the scope are not diagnosed
 as potential ``const``.
 
@@ -18,25 +18,29 @@
   // Declare a variable, which is not ``const`` ...
   int i = 42;
   // but use it as read-only. This means that `i` can be declared ``const``.
-  int result = i * i;
+  int result = i * i;   // Before transformation
+  int const result = i * i; // After transformation
 
-The check can analyzes values, pointers and references but not (yet) pointees:
+The check can analyze values, pointers and references but not (yet) pointees:
 
 .. code-block:: c++
 
   // Normal values like built-ins or objects.
-  int potential_const_int = 42; // 'const int potential_const_int = 42' suggestion.
+  int potential_const_int = 42;   // Before transformation
+  int const potential_const_int = 42; // After transformation
   int copy_of_value = potential_const_int;
 
-  MyClass could_be_const; // 'const MyClass could_be_const' suggestion;
+  MyClass could_be_const;   // Before transformation
+  MyClass const could_be_const; // After transformation
   could_be_const.const_qualified_method();
 
   // References can be declared const as well.
-  int _value = potential_const_int; // 'const int _value' suggestion.
+  int _value = potential_const_int;   // Before transformation
+  int const& reference_value = potential_const_int; // After transformation
   int another_copy = reference_value;
 
   // The similar semantics of pointers are not (yet) analyzed.
-  int *pointer_variable = _const_int; // Not 'const int *pointer_variable' suggestion.
+  int *pointer_variable = _const_int; // _NO_ 'const int *pointer_variable' suggestion.
   int last_copy = *pointer_variable;
 
 The automatic code transformation is only applied to variables that are declared in single
@@ -44,18 +48,20 @@
 `readability-isolate-declaration <../readability/isolate-declaration.html>`_ first.
 
 Note that there is the check
-`cppcoreguidelines-avoid-non-const-global-variables `_
+`cppcoreguidelines-avoid-non-const-global-variables <../cppcoreguidelines/avoid-non-const-global-variables.html>`_
 to enforce ``const`` correctness on all globals.
 
 Known Limitations
 -
 
+The check does not run on `C` code.
+
 The check will not analyze templated variables or variables that are instantiation dependent.
 Different instantiations can result in different ``const`` correctness properties and in general it
-is not possible to find all instantiations of a template. It might be used differently in an
-independent translation unit.
+is not possible to find all instantiations of a template. The template might be used differently in
+an independent translation unit.
 
-Pointees can not be analyzed for constness yet. The following code is shows this limitation.
+Pointees can not be analyzed for constness yet. The following code shows this limitation.
 
 .. code-block:: c++
 
@@ -74,15 +80,35 @@
 Options
 ---
 
-.. option:: AnalyzeValues (default = 1)
+.. option:: AnalyzeValues (default = true)
 
   Enable or disable the analysis of ordinary value variables, like ``int i = 42;``
 
-.. option:: AnalyzeReferences (default = 1)
+  .. code-block:: c++
+
+// Warning
+int i = 42;
+// No warning
+int const i = 42;
+
+// Warning
+int a[] = {42, 42, 42};
+// No warning
+int const a[] = {42, 42, 42};
+
+.. option:: AnalyzeReferences (default = true)
 
   Enable or disable the analysis of reference variables, like ``int  = i;``
 
-.. option:: 

[PATCH] D130793: [clang-tidy] adjust treating of array-of-pointers when 'AnalyzePointers' is deactivated

[Jonas Toth via Phabricator via cfe-commits]

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

- remove bad change from diff


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130793

Files:
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.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-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
@@ -526,18 +526,15 @@
   // CHECK-FIXES: int const p_local1[2]
   for (const int _ref : p_local1) {
   }
+}
 
-  int *p_local2[2] = {_local0[0], _local0[1]};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local2[2]
-  for (const int *con_ptr : p_local2) {
-  }
-
-  int *p_local3[2] = {nullptr, nullptr};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'int *[2]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local3[2]
-  for (const auto *con_ptr : p_local3) {
-  }
+void arrays_of_pointers_are_ignored() {
+  int *np_local0[2] = {nullptr, nullptr};
+  // CHECK-NOT-FIXES: int * const np_local0[2]
+  
+  using intPtr = int*;
+  intPtr np_local1[2] = {nullptr, nullptr};
+  // CHECK-NOT-FIXES: intPtr const np_local1[2]
 }
 
 inline void *operator new(decltype(sizeof(void *)), void *p) { return p; }
@@ -908,41 +905,6 @@
   sizeof(int[++N]);
 }
 
-template 
-struct SmallVectorBase {
-  T data[4];
-  void push_back(const T ) {}
-  int size() const { return 4; }
-  T *begin() { return data; }
-  const T *begin() const { return data; }
-  T *end() { return data + 4; }
-  const T *end() const { return data + 4; }
-};
-
-template 
-struct SmallVector : SmallVectorBase {};
-
-template 
-void EmitProtocolMethodList(T &) {
-  // Note: If the template is uninstantiated the analysis does not figure out,
-  // that p_local0 could be const. Not sure why, but probably bails because
-  // some expressions are type-dependent.
-  SmallVector p_local0;
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'SmallVector' can be declared 'const'
-  // CHECK-FIXES: SmallVector const p_local0
-  SmallVector np_local0;
-  for (const auto *I : Methods) {
-if (I == nullptr)
-  np_local0.push_back(I);
-  }
-  p_local0.size();
-}
-void instantiate() {
-  int *p_local0[4] = {nullptr, nullptr, nullptr, nullptr};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[4]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local0[4]
-  EmitProtocolMethodList(p_local0);
-}
 struct base {
   int member;
 };
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.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-pointer-as-values.cpp
@@ -10,4 +10,65 @@
   double *p_local0 = _local0[1];
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double *' can be declared 'const'
   // CHECK-FIXES: double *const p_local0
+
+  using doublePtr = double*;
+  using doubleArray = double[15];
+  doubleArray np_local1;
+  doublePtr p_local1 = _local1[0];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'doublePtr' (aka 'double *') can be declared 'const'
+  // CHECK-FIXES: doublePtr const p_local1
+}
+
+void range_for() {
+  int np_local0[2] = {1, 2};
+  int *p_local0[2] = {_local0[0], _local0[1]};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[2]' can be declared 'const'
+  // CHECK-FIXES: int *const p_local0[2]
+  for (const int *p_local1 : p_local0) {
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'p_local1' of type 'const int *' can be declared 'const'
+  // CHECK-FIXES: for (const int *const p_local1 : p_local0)
+  }
+
+  int *p_local2[2] = {nullptr, nullptr};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
+  // CHECK-FIXES: int *const p_local2[2]
+  for (const auto *con_ptr : p_local2) {
+  }
+
+}
+
+template 
+struct SmallVectorBase {
+  T data[4];
+  void push_back(const T ) {}
+  int size() const { return 4; }
+  T *begin() { return data; }
+  const T *begin() const { return data; }
+  T *end() { return data + 4; }
+  const T *end() const { return data + 4; }
+};
+
+template 
+struct SmallVector : SmallVectorBase {};
+
+template 
+void EmitProtocolMethodList(T &) {
+  // Note: If the template is uninstantiated the 

[PATCH] D130793: [clang-tidy] adjust treating of array-of-pointers when 'AnalyzePointers' is deactivated

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 454031.
JonasToth marked an inline comment as done.
JonasToth added a comment.

- split patch
- remove unnecessary includes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130793

Files:
  clang-tools-extra/clang-tidy/bugprone/SignalHandlerCheck.cpp
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.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-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
@@ -526,18 +526,15 @@
   // CHECK-FIXES: int const p_local1[2]
   for (const int _ref : p_local1) {
   }
+}
 
-  int *p_local2[2] = {_local0[0], _local0[1]};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local2[2]
-  for (const int *con_ptr : p_local2) {
-  }
-
-  int *p_local3[2] = {nullptr, nullptr};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'int *[2]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local3[2]
-  for (const auto *con_ptr : p_local3) {
-  }
+void arrays_of_pointers_are_ignored() {
+  int *np_local0[2] = {nullptr, nullptr};
+  // CHECK-NOT-FIXES: int * const np_local0[2]
+  
+  using intPtr = int*;
+  intPtr np_local1[2] = {nullptr, nullptr};
+  // CHECK-NOT-FIXES: intPtr const np_local1[2]
 }
 
 inline void *operator new(decltype(sizeof(void *)), void *p) { return p; }
@@ -908,41 +905,6 @@
   sizeof(int[++N]);
 }
 
-template 
-struct SmallVectorBase {
-  T data[4];
-  void push_back(const T ) {}
-  int size() const { return 4; }
-  T *begin() { return data; }
-  const T *begin() const { return data; }
-  T *end() { return data + 4; }
-  const T *end() const { return data + 4; }
-};
-
-template 
-struct SmallVector : SmallVectorBase {};
-
-template 
-void EmitProtocolMethodList(T &) {
-  // Note: If the template is uninstantiated the analysis does not figure out,
-  // that p_local0 could be const. Not sure why, but probably bails because
-  // some expressions are type-dependent.
-  SmallVector p_local0;
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'SmallVector' can be declared 'const'
-  // CHECK-FIXES: SmallVector const p_local0
-  SmallVector np_local0;
-  for (const auto *I : Methods) {
-if (I == nullptr)
-  np_local0.push_back(I);
-  }
-  p_local0.size();
-}
-void instantiate() {
-  int *p_local0[4] = {nullptr, nullptr, nullptr, nullptr};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[4]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local0[4]
-  EmitProtocolMethodList(p_local0);
-}
 struct base {
   int member;
 };
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.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-pointer-as-values.cpp
@@ -10,4 +10,65 @@
   double *p_local0 = _local0[1];
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double *' can be declared 'const'
   // CHECK-FIXES: double *const p_local0
+
+  using doublePtr = double*;
+  using doubleArray = double[15];
+  doubleArray np_local1;
+  doublePtr p_local1 = _local1[0];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'doublePtr' (aka 'double *') can be declared 'const'
+  // CHECK-FIXES: doublePtr const p_local1
+}
+
+void range_for() {
+  int np_local0[2] = {1, 2};
+  int *p_local0[2] = {_local0[0], _local0[1]};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[2]' can be declared 'const'
+  // CHECK-FIXES: int *const p_local0[2]
+  for (const int *p_local1 : p_local0) {
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'p_local1' of type 'const int *' can be declared 'const'
+  // CHECK-FIXES: for (const int *const p_local1 : p_local0)
+  }
+
+  int *p_local2[2] = {nullptr, nullptr};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
+  // CHECK-FIXES: int *const p_local2[2]
+  for (const auto *con_ptr : p_local2) {
+  }
+
+}
+
+template 
+struct SmallVectorBase {
+  T data[4];
+  void push_back(const T ) {}
+  int size() const { return 4; }
+  T *begin() { return data; }
+  const T *begin() const { return data; }
+  T *end() { return data + 4; }
+  const T *end() const { return data + 4; }
+};
+
+template 
+struct SmallVector : 

[PATCH] D132244: [docs] improve documentation for misc-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth created this revision.
JonasToth added reviewers: njames93, aaron.ballman, alexfh, hokein, sammccall.
Herald added a project: All.
JonasToth requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

Improve the documentation for 'misc-const-correctness' to include better 
examples.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D132244

Files:
  clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst

Index: clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
===
--- clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
@@ -4,12 +4,12 @@
 ==
 
 This check implements detection of local variables which could be declared as
-``const``, but are not. Declaring variables as ``const`` is required or recommended by many
+``const`` but are not. Declaring variables as ``const`` is required or recommended by many
 coding guidelines, such as:
 `CppCoreGuidelines ES.25 `_
 and `AUTOSAR C++14 Rule A7-1-1 (6.7.1 Specifiers) `_.
 
-Please note that this analysis is type-based only. Variables that are not modified
+Please note that this check's analysis is type-based only. Variables that are not modified
 but used to create a non-const handle that might escape the scope are not diagnosed
 as potential ``const``.
 
@@ -18,33 +18,37 @@
   // Declare a variable, which is not ``const`` ...
   int i = 42;
   // but use it as read-only. This means that `i` can be declared ``const``.
-  int result = i * i;
+  int result = i * i;   // Before transformation
+  int const result = i * i; // After transformation
 
-The check can analyzes values, pointers and references but not (yet) pointees:
+The check can analyze values, pointers and references but not (yet) pointees:
 
 .. code-block:: c++
 
   // Normal values like built-ins or objects.
-  int potential_const_int = 42; // 'const int potential_const_int = 42' suggestion.
+  int potential_const_int = 42;   // Before transformation
+  int const potential_const_int = 42; // After transformation
   int copy_of_value = potential_const_int;
 
-  MyClass could_be_const; // 'const MyClass could_be_const' suggestion;
+  MyClass could_be_const;   // Before transformation
+  MyClass const could_be_const; // After transformation
   could_be_const.const_qualified_method();
 
   // References can be declared const as well.
-  int _value = potential_const_int; // 'const int _value' suggestion.
+  int _value = potential_const_int;   // Before transformation
+  int const& reference_value = potential_const_int; // After transformation
   int another_copy = reference_value;
 
   // The similar semantics of pointers are not (yet) analyzed.
-  int *pointer_variable = _const_int; // Not 'const int *pointer_variable' suggestion.
+  int *pointer_variable = _const_int; // _NO_ 'const int *pointer_variable' suggestion.
   int last_copy = *pointer_variable;
 
 The automatic code transformation is only applied to variables that are declared in single
 declarations. You may want to prepare your code base with
-`readability-isolate-declaration `_ first.
+`readability-isolate-declaration <../readability/isolate-declaration.html>`_ first.
 
 Note that there is the check
-`cppcoreguidelines-avoid-non-const-global-variables `_
+`cppcoreguidelines-avoid-non-const-global-variables <../cppcoreguidelines/avoid-non-const-global-variables.html>`_
 to enforce ``const`` correctness on all globals.
 
 Known Limitations
@@ -52,10 +56,10 @@
 
 The check will not analyze templated variables or variables that are instantiation dependent.
 Different instantiations can result in different ``const`` correctness properties and in general it
-is not possible to find all instantiations of a template. It might be used differently in an
-independent translation unit.
+is not possible to find all instantiations of a template. The template might be used differently in
+an independent translation unit.
 
-Pointees can not be analyzed for constness yet. The following code is shows this limitation.
+Pointees can not be analyzed for constness yet. The following code shows this limitation.
 
 .. code-block:: c++
 
@@ -74,15 +78,35 @@
 Options
 ---
 
-.. option:: AnalyzeValues (default = 1)
+.. option:: AnalyzeValues (default = true)
 
   Enable or disable the analysis of ordinary value variables, like ``int i = 42;``
 
-.. option:: AnalyzeReferences (default = 1)
+  .. code-block:: c++
+
+// Warning
+int i = 42;
+// No warning
+int const i = 42;
+
+// 

[PATCH] D130793: [clang-tidy] adjust treating of array-of-pointers when 'AnalyzePointers' is deactivated

[Jonas Toth via Phabricator via cfe-commits]

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

- improve test


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130793

Files:
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
@@ -526,18 +526,15 @@
   // CHECK-FIXES: int const p_local1[2]
   for (const int _ref : p_local1) {
   }
+}
 
-  int *p_local2[2] = {_local0[0], _local0[1]};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local2[2]
-  for (const int *con_ptr : p_local2) {
-  }
-
-  int *p_local3[2] = {nullptr, nullptr};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'int *[2]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local3[2]
-  for (const auto *con_ptr : p_local3) {
-  }
+void arrays_of_pointers_are_ignored() {
+  int *np_local0[2] = {nullptr, nullptr};
+  // CHECK-NOT-FIXES: int * const np_local0[2]
+  
+  using intPtr = int*;
+  intPtr np_local1[2] = {nullptr, nullptr};
+  // CHECK-NOT-FIXES: intPtr const np_local1[2]
 }
 
 inline void *operator new(decltype(sizeof(void *)), void *p) { return p; }
@@ -908,41 +905,6 @@
   sizeof(int[++N]);
 }
 
-template 
-struct SmallVectorBase {
-  T data[4];
-  void push_back(const T ) {}
-  int size() const { return 4; }
-  T *begin() { return data; }
-  const T *begin() const { return data; }
-  T *end() { return data + 4; }
-  const T *end() const { return data + 4; }
-};
-
-template 
-struct SmallVector : SmallVectorBase {};
-
-template 
-void EmitProtocolMethodList(T &) {
-  // Note: If the template is uninstantiated the analysis does not figure out,
-  // that p_local0 could be const. Not sure why, but probably bails because
-  // some expressions are type-dependent.
-  SmallVector p_local0;
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'SmallVector' can be declared 'const'
-  // CHECK-FIXES: SmallVector const p_local0
-  SmallVector np_local0;
-  for (const auto *I : Methods) {
-if (I == nullptr)
-  np_local0.push_back(I);
-  }
-  p_local0.size();
-}
-void instantiate() {
-  int *p_local0[4] = {nullptr, nullptr, nullptr, nullptr};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[4]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local0[4]
-  EmitProtocolMethodList(p_local0);
-}
 struct base {
   int member;
 };
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.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-pointer-as-values.cpp
@@ -10,4 +10,65 @@
   double *p_local0 = _local0[1];
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double *' can be declared 'const'
   // CHECK-FIXES: double *const p_local0
+
+  using doublePtr = double*;
+  using doubleArray = double[15];
+  doubleArray np_local1;
+  doublePtr p_local1 = _local1[0];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'doublePtr' (aka 'double *') can be declared 'const'
+  // CHECK-FIXES: doublePtr const p_local1
+}
+
+void range_for() {
+  int np_local0[2] = {1, 2};
+  int *p_local0[2] = {_local0[0], _local0[1]};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[2]' can be declared 'const'
+  // CHECK-FIXES: int *const p_local0[2]
+  for (const int *p_local1 : p_local0) {
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'p_local1' of type 'const int *' can be declared 'const'
+  // CHECK-FIXES: for (const int *const p_local1 : p_local0)
+  }
+
+  int *p_local2[2] = {nullptr, nullptr};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
+  // CHECK-FIXES: int *const p_local2[2]
+  for (const auto *con_ptr : p_local2) {
+  }
+
+}
+
+template 
+struct SmallVectorBase {
+  T data[4];
+  void push_back(const T ) {}
+  int size() const { return 4; }
+  T *begin() { return data; }
+  const T *begin() const { return data; }
+  T *end() { return data + 4; }
+  const T *end() const { return data + 4; }
+};
+
+template 
+struct SmallVector : SmallVectorBase {};
+
+template 
+void EmitProtocolMethodList(T &) {
+  

[PATCH] D130793: [clang-tidy] adjust treating of array-of-pointers when 'AnalyzePointers' is deactivated

[Jonas Toth via Phabricator via cfe-commits]

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

- add test with typedef
- [docs] improve documentation for misc-const-correctness


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130793

Files:
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.cpp
  clang-tools-extra/docs/clang-tidy/checks/misc/const-correctness.rst
  
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.cpp
  clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
@@ -526,18 +526,11 @@
   // CHECK-FIXES: int const p_local1[2]
   for (const int _ref : p_local1) {
   }
+}
 
-  int *p_local2[2] = {_local0[0], _local0[1]};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local2[2]
-  for (const int *con_ptr : p_local2) {
-  }
-
-  int *p_local3[2] = {nullptr, nullptr};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'int *[2]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local3[2]
-  for (const auto *con_ptr : p_local3) {
-  }
+void arrays_of_pointers_are_ignored() {
+  int *np_local0[2] = {nullptr, nullptr};
+  // CHECK-NOT-FIXES: int * const np_local0[2]
 }
 
 inline void *operator new(decltype(sizeof(void *)), void *p) { return p; }
@@ -908,41 +901,6 @@
   sizeof(int[++N]);
 }
 
-template 
-struct SmallVectorBase {
-  T data[4];
-  void push_back(const T ) {}
-  int size() const { return 4; }
-  T *begin() { return data; }
-  const T *begin() const { return data; }
-  T *end() { return data + 4; }
-  const T *end() const { return data + 4; }
-};
-
-template 
-struct SmallVector : SmallVectorBase {};
-
-template 
-void EmitProtocolMethodList(T &) {
-  // Note: If the template is uninstantiated the analysis does not figure out,
-  // that p_local0 could be const. Not sure why, but probably bails because
-  // some expressions are type-dependent.
-  SmallVector p_local0;
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'SmallVector' can be declared 'const'
-  // CHECK-FIXES: SmallVector const p_local0
-  SmallVector np_local0;
-  for (const auto *I : Methods) {
-if (I == nullptr)
-  np_local0.push_back(I);
-  }
-  p_local0.size();
-}
-void instantiate() {
-  int *p_local0[4] = {nullptr, nullptr, nullptr, nullptr};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[4]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local0[4]
-  EmitProtocolMethodList(p_local0);
-}
 struct base {
   int member;
 };
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.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-pointer-as-values.cpp
@@ -10,4 +10,65 @@
   double *p_local0 = _local0[1];
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double *' can be declared 'const'
   // CHECK-FIXES: double *const p_local0
+
+  using doublePtr = double*;
+  using doubleArray = double[15];
+  doubleArray np_local1;
+  doublePtr p_local1 = _local1[0];
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'doublePtr' (aka 'double *') can be declared 'const'
+  // CHECK-FIXES: doublePtr const p_local1
+}
+
+void range_for() {
+  int np_local0[2] = {1, 2};
+  int *p_local0[2] = {_local0[0], _local0[1]};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[2]' can be declared 'const'
+  // CHECK-FIXES: int *const p_local0[2]
+  for (const int *p_local1 : p_local0) {
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'p_local1' of type 'const int *' can be declared 'const'
+  // CHECK-FIXES: for (const int *const p_local1 : p_local0)
+  }
+
+  int *p_local2[2] = {nullptr, nullptr};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
+  // CHECK-FIXES: int *const p_local2[2]
+  for (const auto *con_ptr : p_local2) {
+  }
+
+}
+
+template 
+struct SmallVectorBase {
+  T data[4];
+  void push_back(const T ) {}
+  int size() const { return 4; }
+  T *begin() { return data; }
+  const T *begin() const { return data; }
+  T *end() { return data + 4; }
+  const T *end() const { return data + 4; }
+};
+
+template 
+struct SmallVector : SmallVectorBase {};
+
+template 
+void EmitProtocolMethodList(T &) {
+  // Note: If the template is uninstantiated the 

[PATCH] D130793: [clang-tidy] adjust treating of array-of-pointers when 'AnalyzePointers' is deactivated

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.cpp:16
+void range_for() {
+  int np_local0[2] = {1, 2};
+  int *p_local0[2] = {_local0[0], _local0[1]};

the tests must be exanded to check that typedefs in arrays are handled properly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130793

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

[PATCH] D130793: [clang-tidy] adjust treating of array-of-pointers when 'AnalyzePointers' is deactivated

[Jonas Toth via Phabricator via cfe-commits]

JonasToth created this revision.
JonasToth added reviewers: njames93, aaron.ballman, alexfh.
Herald added subscribers: carlosgalvezp, xazax.hun.
Herald added a project: All.
JonasToth requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.

'misc-const-correctness' previously considered arrays as 'Values' independent 
of the type of the elements.
This is inconsistent with the configuration of the check to disable treating 
pointers as values.
This patch rectifies this inconsistency.

Fixes #56749


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D130793

Files:
  clang-tools-extra/clang-tidy/misc/ConstCorrectnessCheck.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-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-values.cpp
@@ -526,18 +526,11 @@
   // CHECK-FIXES: int const p_local1[2]
   for (const int _ref : p_local1) {
   }
+}
 
-  int *p_local2[2] = {_local0[0], _local0[1]};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local2[2]
-  for (const int *con_ptr : p_local2) {
-  }
-
-  int *p_local3[2] = {nullptr, nullptr};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local3' of type 'int *[2]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local3[2]
-  for (const auto *con_ptr : p_local3) {
-  }
+void arrays_of_pointers_are_ignored() {
+  int *np_local0[2] = {nullptr, nullptr};
+  // CHECK-NOT-FIXES: int * const np_local0[2]
 }
 
 inline void *operator new(decltype(sizeof(void *)), void *p) { return p; }
@@ -908,41 +901,6 @@
   sizeof(int[++N]);
 }
 
-template 
-struct SmallVectorBase {
-  T data[4];
-  void push_back(const T ) {}
-  int size() const { return 4; }
-  T *begin() { return data; }
-  const T *begin() const { return data; }
-  T *end() { return data + 4; }
-  const T *end() const { return data + 4; }
-};
-
-template 
-struct SmallVector : SmallVectorBase {};
-
-template 
-void EmitProtocolMethodList(T &) {
-  // Note: If the template is uninstantiated the analysis does not figure out,
-  // that p_local0 could be const. Not sure why, but probably bails because
-  // some expressions are type-dependent.
-  SmallVector p_local0;
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'SmallVector' can be declared 'const'
-  // CHECK-FIXES: SmallVector const p_local0
-  SmallVector np_local0;
-  for (const auto *I : Methods) {
-if (I == nullptr)
-  np_local0.push_back(I);
-  }
-  p_local0.size();
-}
-void instantiate() {
-  int *p_local0[4] = {nullptr, nullptr, nullptr, nullptr};
-  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[4]' can be declared 'const'
-  // CHECK-FIXES: int *const p_local0[4]
-  EmitProtocolMethodList(p_local0);
-}
 struct base {
   int member;
 };
Index: clang-tools-extra/test/clang-tidy/checkers/misc/const-correctness-pointer-as-values.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-pointer-as-values.cpp
@@ -11,3 +11,57 @@
   // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double *' can be declared 'const'
   // CHECK-FIXES: double *const p_local0
 }
+
+void range_for() {
+  int np_local0[2] = {1, 2};
+  int *p_local0[2] = {_local0[0], _local0[1]};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int *[2]' can be declared 'const'
+  // CHECK-FIXES: int *const p_local0[2]
+  for (const int *p_local1 : p_local0) {
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: variable 'p_local1' of type 'const int *' can be declared 'const'
+  // CHECK-FIXES: for (const int *const p_local1 : p_local0)
+  }
+
+  int *p_local2[2] = {nullptr, nullptr};
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int *[2]' can be declared 'const'
+  // CHECK-FIXES: int *const p_local2[2]
+  for (const auto *con_ptr : p_local2) {
+  }
+
+}
+
+template 
+struct SmallVectorBase {
+  T data[4];
+  void push_back(const T ) {}
+  int size() const { return 4; }
+  T *begin() { return data; }
+  const T *begin() const { return data; }
+  T *end() { return data + 4; }
+  const T *end() const { return data + 4; }
+};
+
+template 
+struct SmallVector : SmallVectorBase {};
+
+template 
+void EmitProtocolMethodList(T &) {
+  // Note: If the template is uninstantiated the analysis does not figure out,
+  // that p_local0 could be const. Not 

[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

[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

[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

[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

[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] D127036: [clang-tidy] Warn about arrays in `bugprone-undefined-memory-manipulation`

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/bugprone/UndefinedMemoryManipulationCheck.cpp:27
 void UndefinedMemoryManipulationCheck::registerMatchers(MatchFinder *Finder) {
-  const auto NotTriviallyCopyableObject =
-  hasType(ast_matchers::hasCanonicalType(
-  pointsTo(cxxRecordDecl(isNotTriviallyCopyable();
+  const auto ArrayOfNotTriviallyCopyable = arrayType(
+  hasElementType(hasDeclaration(cxxRecordDecl(isNotTriviallyCopyable();

you can factor the `hasDeclaration(cxxRecordDecl(isNotTriviallyCopyable()))` 
out to avoid the copy



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/bugprone-undefined-memory-manipulation.cpp:129
 
+  types::Copy ca[10];
+  memset(ca, 0, sizeof(ca));

could you please add a test with a typedef to array to ensure that those are 
matched as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127036

___
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

[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 const-transformation for misc-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- fix: remove clangAnalysis link in cppcoreguidelines and add it in misc


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; // 

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

[Jonas Toth via Phabricator via cfe-commits]

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

- refactor: rename check to 'misc-const-correctness' and adjust the tests 
accordingly
- docs: adjust release notes and adjust check docs slightly


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/cppcoreguidelines/CMakeLists.txt
  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 

[PATCH] D119481: run-clang-tidy: Fix infinite loop on windows

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Sorry for the long delay, i simply forgot.
The patch is commited! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119481

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

[PATCH] D119481: run-clang-tidy: Fix infinite loop on windows

[Jonas Toth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG3f0f20366622: run-clang-tidy: Fix infinite loop on windows 
(authored by JonasToth).
Herald added a project: All.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119481

Files:
  clang-tools-extra/clang-tidy/tool/run-clang-tidy.py


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -73,13 +73,14 @@
 
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
-  result = './'
+  result = os.path.realpath('./')
   while not os.path.isfile(os.path.join(result, path)):
-if os.path.realpath(result) == '/':
+parent = os.path.dirname(result)
+if result == parent:
   print('Error: could not find compilation database.')
   sys.exit(1)
-result += '../'
-  return os.path.realpath(result)
+result = parent
+  return result
 
 
 def make_absolute(f, directory):


Index: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
===
--- clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
+++ clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
@@ -73,13 +73,14 @@
 
 def find_compilation_database(path):
   """Adjusts the directory until a compilation database is found."""
-  result = './'
+  result = os.path.realpath('./')
   while not os.path.isfile(os.path.join(result, path)):
-if os.path.realpath(result) == '/':
+parent = os.path.dirname(result)
+if result == parent:
   print('Error: could not find compilation database.')
   sys.exit(1)
-result += '../'
-  return os.path.realpath(result)
+result = parent
+  return result
 
 
 def make_absolute(f, directory):
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

ok. then i will continue under `misc-const-correctness`. thank you for the 
clarifications!


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> Sorry, for my own sanity, I've stopped reviewing C++ Core Guideline reviews 
> for their diagnostic behavior until the guideline authors put effort into 
> specifying realistic enforcement guidance.

And how can we continue now? I fear that this is effectively the death of this 
check. :/
Isn't the "make this const" good enough?


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

ping @aaron.ballman @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] D120331: [clang-tidy][run-clang-tidy.py] Add --config-file= option

[Jonas Toth via Phabricator via cfe-commits]

JonasToth accepted this revision.
JonasToth added a comment.
This revision is now accepted and ready to land.
Herald added a project: All.

LGTM, but please adjust the naming nit to a better name.

do you have commit rights? Otherwise someone (e.g. me) could commit on your 
behalf, of course with a remark that its your contribution :)




Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:228
   'default')
-  parser.add_argument('-config', default=None,
+  group = parser.add_mutually_exclusive_group()
+  group.add_argument('-config', default=None,

nit: i think `config_group` or so would be better to show that its only 
config-related. This name is too generic i feel.



Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:236
   'each source file in its parent directories.')
+  parser.add_argument('-config-file', default=None,
+  help='Specify the path of .clang-tidy or custom config'

SAtacker wrote:
> JonasToth wrote:
> > please ensure that those option exclude each other. right now it could be 
> > used with both options which is a contradiction for `clang-tidy`
> Do you mean mutual exclusion? From the docs: 
> https://docs.python.org/3/library/argparse.html#mutual-exclusion
Yes, thanks :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120331

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

[PATCH] D120331: [clang-tidy][run-clang-tidy.py] Add --config-file= option

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:236
   'each source file in its parent directories.')
+  parser.add_argument('-config-file', default=None,
+  help='Specify the path of .clang-tidy or custom config'

please ensure that those option exclude each other. right now it could be used 
with both options which is a contradiction for `clang-tidy`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120331

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

[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Jonas Toth via Phabricator via cfe-commits]

JonasToth abandoned this revision.
JonasToth added a comment.
Herald added a subscriber: mgehre.

won't happen anymore realistically.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54141

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

[PATCH] D119481: run-clang-tidy: Fix infinite loop on windows

[Jonas Toth via Phabricator via cfe-commits]

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

LGTM with a small nit




Comment at: clang-tools-extra/clang-tidy/tool/run-clang-tidy.py:69
   while not os.path.isfile(os.path.join(result, path)):
-if os.path.realpath(result) == '/':
+parent = os.path.dirname(result)
+if result == parent:

please delete the whitespace at the end of the line


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119481

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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Another full run on LLVM with:

  $ ./clang-tools-extra/clang-tidy/tool/run-clang-tidy.py \
-clang-tidy-binary 
~/software/llvm-project/build_clang_tidy/bin/clang-tidy \
-checks="-*,cppcoreguidelines-const-correctness" \
-config "{CheckOptions: [{key: 
'cppcoreguidelines-const-correctness.TransformValues', value: 1}, {key: 
'cppcoreguidelines-const-correctness.TransformReferences', value: 1}, {key: 
'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: true}, {key: 
'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 
true}]}" \
-fix 2>&1 | tee ../transformation.log

Yields to this: ` 3822 files changed, 159256 insertions(+), 159256 deletions(-)`
and requires manual changes:
F22182704: to_fix.patch 
 23 files changed, 45 insertions(+), 45 deletions(-)

the issues were:

- missing `{}` initialization after a variable became `const` if a destructor 
was missing
- the analysis got confused for some `insert(llvm::Function* f)` calls and made 
the objects `const`. Those are false positives, but with a really low rate

after applying the patch `ninja check-all` is successfull.


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked 3 inline comments as done.
JonasToth added inline comments.



Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt:33
   LINK_LIBS
+  clangAnalysis
   clangTidy

sammccall wrote:
> JonasToth wrote:
> > njames93 wrote:
> > > Will this work under clangd as I don't think that links in the 
> > > clangAnalysis libraries @sammccall?
> > I did not check if it works and `clangd` does not explicitly link to 
> > `Analysis`, but only to the checks.
> > Doesn't the `LINK_LIBS` induce transitive dependency resolution for linking?
> I don't know whether you need to add it everywhere, I suspect transitive is 
> OK.
> 
> In any case this isn't a new dependency, Sema depends on Analysis and clangd 
> depends on Sema.
i verified that the check works with clangd without any additional build 
settings.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:29
+  ast_matchers::internal::Matcher, InnerMatcher) {
+  return ast_matchers::internal::matchesFirstInPointerRange(
+  InnerMatcher, Node.decl_begin(), Node.decl_end(), Finder, Builder);

sammccall wrote:
> sammccall wrote:
> > This returns an iterator (i.e. a pointer), which is being converted to a 
> > boolean.
> > 
> > i.e. it's always returning true. I think this is why you're seeing nullptr 
> > crashes on Variable.
> this is depending on `::internal` details, which doesn't seem OK.
> I think you'd need to find another way to do it, or move this to ASTMatchers 
> (in a separate patch).
it is common to write custom matchers in clang-tidy first and use the internal 
matcher apis as well in clang-tidy.

```
$ cd llvm-project/clang-tools-extra/clang-tidy
$ git grep "internal::" | wc -l
86
```

Usually, they are extracted once they are generally useful. And many checks 
introduce custom matchers for better readability.
I can extract this matcher later if you want, but right now with the longevity 
of the patch and it being common practice in clang-tidy I am opposed to it.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:29
+  ast_matchers::internal::Matcher, InnerMatcher) {
+  return ast_matchers::internal::matchesFirstInPointerRange(
+  InnerMatcher, Node.decl_begin(), Node.decl_end(), Finder, Builder);

JonasToth wrote:
> sammccall wrote:
> > sammccall wrote:
> > > This returns an iterator (i.e. a pointer), which is being converted to a 
> > > boolean.
> > > 
> > > i.e. it's always returning true. I think this is why you're seeing 
> > > nullptr crashes on Variable.
> > this is depending on `::internal` details, which doesn't seem OK.
> > I think you'd need to find another way to do it, or move this to 
> > ASTMatchers (in a separate patch).
> it is common to write custom matchers in clang-tidy first and use the 
> internal matcher apis as well in clang-tidy.
> 
> ```
> $ cd llvm-project/clang-tools-extra/clang-tidy
> $ git grep "internal::" | wc -l
> 86
> ```
> 
> Usually, they are extracted once they are generally useful. And many checks 
> introduce custom matchers for better readability.
> I can extract this matcher later if you want, but right now with the 
> longevity of the patch and it being common practice in clang-tidy I am 
> opposed to it.
thank you for that hint! that would explain the issues indeed. I try to verify 
your proposed fix by re-transforming LLVM.
some other subscribers checked their own codebases as well. this would give us 
more confidence too.

if the crash is still there, I would like to keep the 'safety-if' in `check` 
and try to find the issue with the matcher.
The path @njames93 linked seemed like it would help there a lot.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:84
+  compoundStmt(
+  findAll(declStmt(allOf(containsDeclaration2(
+ LocalValDecl.bind("local-value")),

njames93 wrote:
> `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.
switching to `forEachDescendant`, both seem to work.
I am unsure why I used `findAll`, but I know that I started out with 
`forEachDescendant` for some bits of the check and it was insufficent.
But it could very well be, that these pieces are now in the `ExprMutAnalyzer`.



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;

njames93 wrote:
> 

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 410179.
JonasToth marked 14 inline comments as done.
JonasToth added a comment.

- address reviews comments, part 1
- Merge branch 'main' into feature_rebase_const_transform_20210808
- fixing iterator-to-bool conversion and addressing other more review comments


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/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-unaligned.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-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/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,958 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: true}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: false}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: false}, \
+// RUN:   ]}" -- -fno-delayed-template-parsing
+
+// --- Provide test samples for primitive builtins 

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt:33
   LINK_LIBS
+  clangAnalysis
   clangTidy

njames93 wrote:
> Will this work under clangd as I don't think that links in the clangAnalysis 
> libraries @sammccall?
I did not check if it works and `clangd` does not explicitly link to 
`Analysis`, but only to the checks.
Doesn't the `LINK_LIBS` induce transitive dependency resolution for linking?



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;

njames93 wrote:
> This sounds like a bug that should be raised with ASTMatchers, if its 
> reproducible.
I found no way to reproduce it (yet).
The crashes occured during verification on big projects and `run-clang-tidy` 
kind of obfuscated where the crash happened.

This is something for the iron out part of the check I think.


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] D118927: [clang-tidy] Fix invalid fix-it for cppcoreguidelines-prefer-member-initializer

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-prefer-member-initializer.cpp:550
+  // CHECK-FIXES-NEXT: }
+  PR53515(PR53515 &) : M() {
+M = Other.M;

could you please add a test-case for initializer lists, like `std::vector`? Or 
is this not supported?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118927

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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D54943#3292552 , @0x8000- 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.


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



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

LegalizeAdulthood wrote:
> JonasToth wrote:
> > JonasToth wrote:
> > > 0x8000- wrote:
> > > > LegalizeAdulthood wrote:
> > > > > Shouldn't this validate that the `const` was placed in the correct 
> > > > > position?
> > > > > e.g. `const double *` is a different meaning from `double *const`
> > > > > 
> > > > > Apply to all the other `CHECK-FIXES` as well
> > > > Can we have the checker merged in first, then we can worry about the 
> > > > automatic fixer?
> > > the checker is its own utility with its own tests and proper test 
> > > coverage.
> > > yes `const double*` and `double* const` are different and are correctly 
> > > inserted, but that is not tested here, but here: 
> > > https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
> > > 
> > > Similar to the actual `const` analysis. That has its own test-suite 
> > > (https://github.com/llvm/llvm-project/blob/main/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp)
> > > 
> > > The tests here are concerned with the diagnostics and "real" code. 
> > > Afterall, this functionality is too complex to have all of it checked 
> > > with these kind of tests.
> > > I think the separate testing in specialized unit-tests (as is now) for 
> > > the specialized functions is the right approach and the `CHECK-FIXES` are 
> > > not necessary in this instance, maybe even bad, because it makes the 
> > > tests unclearer.
> > sry: The _fixer_ is its own utility ...
> > 
> > Additionally: The test is run on big projects with transformation (LLVM, 
> > some Qt Stuff).
> > First everything is transformed and then recompiled. The compiler tells 
> > what was incorrectly inserted :)
> > 
> > Thats part of the testing too.
> > The tests here are concerned with the diagnostics and "real" code.
> 
> OK, great!  I didn't realize it was covered by unit tests,
> which is perfectly fine with me `:)`
Perfect!

Yeah, the check and patches here are a bit all over the place, which is another 
reason why a merge would be really great :D


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked 2 inline comments as done.
JonasToth added inline comments.



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

JonasToth wrote:
> 0x8000- wrote:
> > LegalizeAdulthood wrote:
> > > Shouldn't this validate that the `const` was placed in the correct 
> > > position?
> > > e.g. `const double *` is a different meaning from `double *const`
> > > 
> > > Apply to all the other `CHECK-FIXES` as well
> > Can we have the checker merged in first, then we can worry about the 
> > automatic fixer?
> the checker is its own utility with its own tests and proper test coverage.
> yes `const double*` and `double* const` are different and are correctly 
> inserted, but that is not tested here, but here: 
> https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
> 
> Similar to the actual `const` analysis. That has its own test-suite 
> (https://github.com/llvm/llvm-project/blob/main/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp)
> 
> The tests here are concerned with the diagnostics and "real" code. Afterall, 
> this functionality is too complex to have all of it checked with these kind 
> of tests.
> I think the separate testing in specialized unit-tests (as is now) for the 
> specialized functions is the right approach and the `CHECK-FIXES` are not 
> necessary in this instance, maybe even bad, because it makes the tests 
> unclearer.
sry: The _fixer_ is its own utility ...

Additionally: The test is run on big projects with transformation (LLVM, some 
Qt Stuff).
First everything is transformed and then recompiled. The compiler tells what 
was incorrectly inserted :)

Thats part of the testing too.


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked 2 inline comments as done.
JonasToth added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp:16
+  int value_int = 42;
+  // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'value_int' of type 'int' 
can be declared 'const'
+}

LegalizeAdulthood wrote:
> `CHECK-FIXES`?
see my comment in the other test.



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

0x8000- wrote:
> LegalizeAdulthood wrote:
> > Shouldn't this validate that the `const` was placed in the correct position?
> > e.g. `const double *` is a different meaning from `double *const`
> > 
> > Apply to all the other `CHECK-FIXES` as well
> Can we have the checker merged in first, then we can worry about the 
> automatic fixer?
the checker is its own utility with its own tests and proper test coverage.
yes `const double*` and `double* const` are different and are correctly 
inserted, but that is not tested here, but here: 
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp

Similar to the actual `const` analysis. That has its own test-suite 
(https://github.com/llvm/llvm-project/blob/main/clang/unittests/Analysis/ExprMutationAnalyzerTest.cpp)

The tests here are concerned with the diagnostics and "real" code. Afterall, 
this functionality is too complex to have all of it checked with these kind of 
tests.
I think the separate testing in specialized unit-tests (as is now) for the 
specialized functions is the right approach and the `CHECK-FIXES` are not 
necessary in this instance, maybe even bad, because it makes the tests 
unclearer.


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

ping @aaron.ballman just in case the patch is now down on the list, I am sorry 
for my high latency :(




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h:29
+  : ClangTidyCheck(Name, Context),
+AnalyzeValues(Options.get("AnalyzeValues", 1)),
+AnalyzeReferences(Options.get("AnalyzeReferences", 1)),

njames93 wrote:
> Can all these options be defaulted to Boolean values, as the options parser 
> has special parsing logic for bool
you are right. I updated it.


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- use boolean for options
- fix snafoo on `arc` usage


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/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-unaligned.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-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/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,958 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
+// 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 

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

[Jonas Toth via Phabricator via cfe-commits]

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

- use boolean for option parsing


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/cppcoreguidelines/ConstCorrectnessCheck.h


Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
@@ -26,13 +26,13 @@
 public:
   ConstCorrectnessCheck(StringRef Name, ClangTidyContext *Context)
   : ClangTidyCheck(Name, Context),
-AnalyzeValues(Options.get("AnalyzeValues", 1)),
-AnalyzeReferences(Options.get("AnalyzeReferences", 1)),
-WarnPointersAsValues(Options.get("WarnPointersAsValues", 0)),
-TransformValues(Options.get("TransformValues", 1)),
-TransformReferences(Options.get("TransformReferences", 1)),
-TransformPointersAsValues(Options.get("TransformPointersAsValues", 0)) 
{
-  }
+AnalyzeValues(Options.get("AnalyzeValues", true)),
+AnalyzeReferences(Options.get("AnalyzeReferences", true)),
+WarnPointersAsValues(Options.get("WarnPointersAsValues", false)),
+TransformValues(Options.get("TransformValues", true)),
+TransformReferences(Options.get("TransformReferences", true)),
+TransformPointersAsValues(
+Options.get("TransformPointersAsValues", false)) {}
 
   // The rules for C and 'const' are different and incompatible for this check.
   bool isLanguageVersionSupported(const LangOptions ) const override {


Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
@@ -26,13 +26,13 @@
 public:
   ConstCorrectnessCheck(StringRef Name, ClangTidyContext *Context)
   : ClangTidyCheck(Name, Context),
-AnalyzeValues(Options.get("AnalyzeValues", 1)),
-AnalyzeReferences(Options.get("AnalyzeReferences", 1)),
-WarnPointersAsValues(Options.get("WarnPointersAsValues", 0)),
-TransformValues(Options.get("TransformValues", 1)),
-TransformReferences(Options.get("TransformReferences", 1)),
-TransformPointersAsValues(Options.get("TransformPointersAsValues", 0)) {
-  }
+AnalyzeValues(Options.get("AnalyzeValues", true)),
+AnalyzeReferences(Options.get("AnalyzeReferences", true)),
+WarnPointersAsValues(Options.get("WarnPointersAsValues", false)),
+TransformValues(Options.get("TransformValues", true)),
+TransformReferences(Options.get("TransformReferences", true)),
+TransformPointersAsValues(
+Options.get("TransformPointersAsValues", false)) {}
 
   // The rules for C and 'const' are different and incompatible for this check.
   bool isLanguageVersionSupported(const LangOptions ) const override {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonas Toth via Phabricator via cfe-commits]

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

- fix: adjust unit test for new array-pointer condition in range-based for loops


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/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-unaligned.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-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/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,958 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
+// 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 

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

[Jonas Toth via Phabricator via cfe-commits]

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

- fix release note ordering


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/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-unaligned.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
  clang/lib/Analysis/ExprMutationAnalyzer.cpp

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/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,958 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
+// 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
+} // namespace foo
+
+// FIXME: Similary to 'static' globals, anonymous globals are not matched and analyzed.
+namespace {
+int np_anonymous_global;
+int p_anonymous_global = 43;
+} // namespace
+
+// Lambdas should be ignored, because they do not follow the normal variable
+// semantic (e.g. the type is only known to the compiler).
+void lambdas() {
+  auto Lambda = [](int i) { return i < 0; };
+}
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 400291.
JonasToth marked an inline comment as done.
JonasToth added a comment.

- improve documentation a bit


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/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-unaligned.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
  clang/lib/Analysis/ExprMutationAnalyzer.cpp

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/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,958 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
+// 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
+} // namespace foo
+
+// FIXME: Similary to 'static' globals, anonymous globals are not matched and analyzed.
+namespace {
+int np_anonymous_global;
+int p_anonymous_global = 43;
+} // namespace
+
+// Lambdas should be ignored, because they do not follow the normal variable
+// semantic (e.g. the type is only known to the compiler).
+void lambdas() {
+  auto Lambda = [](int i) { return i < 0; };
+}
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned 

[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] = {_local0[0], _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::value)),
8hasRangeStmt(DeclStmtToNonRefToArray),
9hasRangeInit(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

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

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 400288.
JonasToth marked 2 inline comments as done.
JonasToth added a comment.

- Merge branch 'main' into feature_rebase_const_transform_20210808
- fix: address other review comments
- remove one false positive with pointers in a range-based for loop on arrays


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/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-unaligned.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
  clang/lib/Analysis/ExprMutationAnalyzer.cpp

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/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,958 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
+// 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
+} // namespace foo
+
+// FIXME: Similary to 'static' globals, anonymous globals are not matched and analyzed.
+namespace {
+int np_anonymous_global;
+int p_anonymous_global = 43;
+} // namespace
+
+// Lambdas should be ignored, because they do not follow the normal variable
+// semantic (e.g. the type is only known to the compiler).
+void lambdas() {
+  auto Lambda = [](int i) { return i < 0; };
+}
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // 

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h:31
+WarnPointersAsValues(Options.get("WarnPointersAsValues", 0)),
+TransformValues(Options.get("TransformValues", 1)),
+TransformReferences(Options.get("TransformReferences", 1)),

aaron.ballman wrote:
> JonasToth wrote:
> > aaron.ballman wrote:
> > > JonasToth wrote:
> > > > tiagoma wrote:
> > > > > JonasToth wrote:
> > > > > > Deactivating the transformations by default, as they are not ready 
> > > > > > for production yet.
> > > > > Ok. This got me off-guard. I would rather have this on.
> > > > the transformations were buggy and required a lot of fix-up. This might 
> > > > not be the case, but i would rather have it non-destructive by default 
> > > > because i think many people will throw this check at their code-base.
> > > > 
> > > > if the transformations are solid already we can activate it again (or 
> > > > maybe references only or so).
> > > Are we still intending to be conservative here? This defaults some of the 
> > > Transform to 1 instead of 0, but that's different than what the 
> > > documentation currently reads.
> > good question. given the current result of it being quiet solid we might 
> > have it on? But I am fine with it being off.
> > I am not sure if it would induce too much transformations to handle. I am 
> > unsure.
> Based on the test coverage we have, I think I'd be fine with it being on by 
> default, but I do think the most conservative approach would be to be off by 
> default. I don't have a strong opinion either way aside from "the 
> documentation should match the code."
I matched the documentation to that its on by default. It is kinda what the 
check promises, so I think having it off is not user-friendly.
I removed the `Experimental` note for it as well. It seems to be "good enough" 
that it does not interfere with everyday programming and even complex things.
The different validation runs seem to agree somewhat (even though there are 
still issues).



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp:40
+void define_locals(T np_arg0, T _arg1, int np_arg2) {
+  T np_local0 = 0;
+  int p_local1 = 42;

aaron.ballman wrote:
> JonasToth wrote:
> > aaron.ballman wrote:
> > > Are we being conservative here about the diagnostic? It seems like the 
> > > instantiations in the TU would mean that this could be declared `const`.
> > Yes, the check ignores templates when matching. The reasoning behind this 
> > was: too much at once. TU-local templates should be transformable, but it 
> > must be ensured that all instantiations can be `const`. Probably the best 
> > solution would be that the template uses concepts and the concepts allow 
> > `const` at this place.
> > This is hopefully follow-up :)
> Okay, that sounds good to me!
Yes we are conservative. Once its a templated variable the analysis bails out. 
Non-templated variables in a template are analyzed though.

The 'check all instantiations' are not implemented, but there is already 
something that goes a little bit in that direction.
Line 118: `TemplateDiagnosticsCache.contains(Variable->getBeginLoc()))`, which 
is necessary to deduplicate emitted diagnostics in templates.
Similarly, TU-local templates could be checked all-together and if the variable 
could be `const` every single time, the diagnostic can be emitted.

But I feel that checking the concepts would be a much better approach to this. 
This would include the non-local templates as well. At the same time, most 
templates don't use them yet.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp:114
+
+void casts() {
+  decltype(sizeof(void *)) p_local0 = 42;

aaron.ballman wrote:
> The test really doesn't have anything to do with casts, does it?
Thats correct :D Renamed


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 400280.
JonasToth marked 10 inline comments as done.
JonasToth added a comment.

- Merge branch 'main' into feature_rebase_const_transform_20210808
- fix: fixes for most review comments


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/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-unaligned.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,953 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
+// 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
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+// Lambdas should be ignored, because they do not follow the normal variable
+// semantic (e.g. the type is only known to the compiler).
+void lambdas() {
+  auto Lambda = [](int i) { return i < 0; };
+}
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  int np_local0 = 42;
+
+  {
+int p_local1 = 42;
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared 'const'
+np_local0 *= 2;
+  }
+}
+
+void ignore_reference_to_pointers() {
+  int *np_local0 = nullptr;
+  int *_local1 = np_local0;
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = _local0;
+  int *const p1_np_local0 = _local0;
+
+  int np_local1 = 42;
+  const int *const p0_np_local1 = _local1;
+  int *const p1_np_local1 = _local1;
+  

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h:31
+WarnPointersAsValues(Options.get("WarnPointersAsValues", 0)),
+TransformValues(Options.get("TransformValues", 1)),
+TransformReferences(Options.get("TransformReferences", 1)),

aaron.ballman wrote:
> JonasToth wrote:
> > tiagoma wrote:
> > > JonasToth wrote:
> > > > Deactivating the transformations by default, as they are not ready for 
> > > > production yet.
> > > Ok. This got me off-guard. I would rather have this on.
> > the transformations were buggy and required a lot of fix-up. This might not 
> > be the case, but i would rather have it non-destructive by default because 
> > i think many people will throw this check at their code-base.
> > 
> > if the transformations are solid already we can activate it again (or maybe 
> > references only or so).
> Are we still intending to be conservative here? This defaults some of the 
> Transform to 1 instead of 0, but that's different than what the documentation 
> currently reads.
good question. given the current result of it being quiet solid we might have 
it on? But I am fine with it being off.
I am not sure if it would induce too much transformations to handle. I am 
unsure.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp:40
+void define_locals(T np_arg0, T _arg1, int np_arg2) {
+  T np_local0 = 0;
+  int p_local1 = 42;

aaron.ballman wrote:
> Are we being conservative here about the diagnostic? It seems like the 
> instantiations in the TU would mean that this could be declared `const`.
Yes, the check ignores templates when matching. The reasoning behind this was: 
too much at once. TU-local templates should be transformable, but it must be 
ensured that all instantiations can be `const`. Probably the best solution 
would be that the template uses concepts and the concepts allow `const` at this 
place.
This is hopefully follow-up :)



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp:120
+
+// taken from http://www.cplusplus.com/reference/type_traits/integral_constant/
+template 

aaron.ballman wrote:
> This is a problem -- I see no license on that site, but I do see "All rights 
> reserved". I don't think we should copy from there.
True. I will try to copy from libc++ instead and remove the reference :)


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D54943#3194643 , @aaron.ballman 
wrote:

> Thanks for pinging on this, it had fallen off my radar! @JonasToth -- did you 
> ever get the chance to talk with @cjdb about the overlap mentioned in 
> https://reviews.llvm.org/D54943#2950100? (The other review also seems to have 
> stalled out, so I'm not certain where things are at.)

We talked only short in the review, but I am not sure if there is such a big 
overlap between the checks. The other check looks at arguments, which is 
something this check currently skips completely.
I think, this check should continue to avoid this analysis, too.

The underlying `ExprMutAnalyzer` is reusable (I think so, if not that should be 
fixed). But my current understanding is, that the argument checker already 
knows the values are `const`, because it matches on `const &` parameters.
IMHO keeping the checks independent would be better. Otherwise a future 
combined check can be aliased with specific configurations.


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.
Herald added a subscriber: carlosgalvezp.

@aaron.ballman ping for review :)

I do not intend to fix the raised issues with this revision. I think it is more 
valuable to get this check in and get more user-feedback. But the found bugs 
are on the todo-list!


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

ping


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I further analyzer the false positive and its probably not so simple to fix.

'NoOp' is always used to add qualifiers. This is true for `const` and 
`__unaligned`, but there seems to be no further information to disambiguate 
those.
Maybe this requires even something in the front-end to change. WDYT 
@aaron.ballman ?


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- add reproducer for '__unaligned' bug


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/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-unaligned.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,953 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
+// 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
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+// Lambdas should be ignored, because they do not follow the normal variable
+// semantic (e.g. the type is only known to the compiler).
+void lambdas() {
+  auto Lambda = [](int i) { return i < 0; };
+}
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  int np_local0 = 42;
+
+  {
+int p_local1 = 42;
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared 'const'
+np_local0 *= 2;
+  }
+}
+
+void ignore_reference_to_pointers() {
+  int *np_local0 = nullptr;
+  int *_local1 = np_local0;
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = _local0;
+  int *const p1_np_local0 = _local0;
+
+  int np_local1 = 42;
+  const int *const p0_np_local1 = _local1;
+  int *const p1_np_local1 = _local1;
+  *p1_np_local0 = 43;
+
+  int np_local2 = 42;
+  function_inout_pointer(_local2);
+
+  // Prevents const.
+  int 

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D54943#2956218 , @tiagoma wrote:

> I am getting false positives with
>
>   struct S{};
>   
>   void f(__unaligned S*);
>   
>   void scope()
>   {
>   S s;
>   f();
>   }

This godbolt link has the AST for the example.
https://godbolt.org/z/8EvsM3Eqe

The `ExprMutAnalyzer` ignores 'NoOp' casts as modifications, because in our 
testing the 'adding const' to an argument was a NoOp-cast. In this case, this 
is a NoOp-Cast, too, but not adding const.
This shadows marking the pointer as mutated and results in the false positive.

I would leave this out of this patch, but make a separate fix. Other forms of 
NoOp-casting are affected too.
https://bugs.llvm.org/show_bug.cgi?id=51756


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- Merge branch 'main' into feature_rebase_const_transform_20210808


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/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,953 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
+// 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
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+// Lambdas should be ignored, because they do not follow the normal variable
+// semantic (e.g. the type is only known to the compiler).
+void lambdas() {
+  auto Lambda = [](int i) { return i < 0; };
+}
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  int np_local0 = 42;
+
+  {
+int p_local1 = 42;
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared 'const'
+np_local0 *= 2;
+  }
+}
+
+void ignore_reference_to_pointers() {
+  int *np_local0 = nullptr;
+  int *_local1 = np_local0;
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = _local0;
+  int *const p1_np_local0 = _local0;
+
+  int np_local1 = 42;
+  const int *const p0_np_local1 = _local1;
+  int *const p1_np_local1 = _local1;
+  *p1_np_local0 = 43;
+
+  int np_local2 = 42;
+  function_inout_pointer(_local2);
+
+  // Prevents const.
+  int np_local3 = 42;
+  out = _local3; // This returns and invalid 

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 370807.
JonasToth marked 2 inline comments as done.
JonasToth added a comment.

- minor adjustments for comments


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/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,953 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
+// 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
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+// Lambdas should be ignored, because they do not follow the normal variable
+// semantic (e.g. the type is only known to the compiler).
+void lambdas() {
+  auto Lambda = [](int i) { return i < 0; };
+}
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  int np_local0 = 42;
+
+  {
+int p_local1 = 42;
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared 'const'
+np_local0 *= 2;
+  }
+}
+
+void ignore_reference_to_pointers() {
+  int *np_local0 = nullptr;
+  int *_local1 = np_local0;
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = _local0;
+  int *const p1_np_local0 = _local0;
+
+  int np_local1 = 42;
+  const int *const p0_np_local1 = _local1;
+  int *const p1_np_local1 = _local1;
+  *p1_np_local0 = 43;
+
+  int np_local2 = 42;
+  function_inout_pointer(_local2);
+
+  // Prevents const.
+  int np_local3 = 42;
+  out = _local3; // This returns and 

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

thanks for your testing! i will look at the `__unaligned` issue, not sure if 
clang supports it, its an MSVC extension, is it?

Did the transformations produce issues and approximately how much code did you 
check? I would like to get a better feeling for its quality before enabling it 
by default.




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:168
+*Variable, *Result.Context, DeclSpec::TQ_const,
+QualifierTarget::Value, QualifierPolicy::Right)) {
+  Diag << *Fix;

tiagoma wrote:
> The core guidelines suggests the use of west const. Could this be made the 
> default?
> https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rl-const
https://reviews.llvm.org/D69764


I was hoping for this feature to handle this instead.
It is extremely complicated to figure out where to insert the `const`, because 
pointer, pointee, value, templates, macros and so on and so forth.
I implemented it such that it is always correct (the right side) and hoped that 
clang-format can fix it up afterwards (`clang-tidy -fix -format`)



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h:31
+WarnPointersAsValues(Options.get("WarnPointersAsValues", 0)),
+TransformValues(Options.get("TransformValues", 1)),
+TransformReferences(Options.get("TransformReferences", 1)),

tiagoma wrote:
> JonasToth wrote:
> > Deactivating the transformations by default, as they are not ready for 
> > production yet.
> Ok. This got me off-guard. I would rather have this on.
the transformations were buggy and required a lot of fix-up. This might not be 
the case, but i would rather have it non-destructive by default because i think 
many people will throw this check at their code-base.

if the transformations are solid already we can activate it again (or maybe 
references only or 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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D54943#2947902 , @cjdb wrote:

> @JonasToth, @aaron.ballman suspects that there's enough overlap between this 
> patch and D107873  to consider merging the 
> two efforts. I'm happy to collaborate, but we should probably check that our 
> goals are aligned. Are you active on the LLVM Discord? We could probably 
> discuss a fair bit of this offline and then ping Aaron after we work out 
> whether or not to do this.

Hey @cjdb . I am usually not on discord (rather IRC, but i am not so active 
anymore) but we can chat for sure. I will not be available until monday and I 
live in the berlin timezone. You can write me a mail 
(developmentjonas-toth.eu) and then we can figure out a time :)


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:145
+  /// const emit multiple warnings otherwise.
+  const bool IsNormalVariableInTemplate = Function->isTemplateInstantiation();
+  if (IsNormalVariableInTemplate &&

this check can happen before the heavy lifting.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:78
+
+  Suggest adding ``const`` to unmodified local variables.
+

Add a note on the transformation capability, too.


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

The check in the latest form can properly transform `llvm/lib` and `clang/` and 
requires 28 manual fixes afterwards (values and references are transformed, no 
other modifications).
 Some are good and not an issue with the analysis itself, some may or may not 
be an issue with the analysis itself. I think it is more a template meta 
programming thing.

Here is the branch that contains the changes: 
https://github.com/JonasToth/llvm-project/tree/transformation/2021-august-15-revision2

Short Stats:

- llvm/lib transformation: `1724 files changed, 62110 insertions(+), 62110 
deletions`; `26 manual fixes`
- clang/ transformation: `819 files changed, 24166 insertions(+), 24166 
deletions`; `2 manual fixes`, one fix solves a ternary-operator issue where 
only one branch got `const`, the other one adds default construction

I consider all 27 of them as false positives, but they might be bad 
const-correctness of the library code as well.
Thats good enough?

@0x8000- @tiagoma If you have time it would be great if you could check the 
transformation on your code bases.
Everyone else is welcome to try it out too!


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 const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- fix: auto and type-dependent initializers fixed by ignoring them correctly in 
the matcher
- fix docs on limitations
- fix: deduplicate diagnostics from template instantiations


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/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-templates.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,953 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
+// 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
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+// Lambdas should be ignored, because they do not follow the normal variable
+// semantic (e.g. the type is only known to the compiler).
+void lambdas() {
+  auto Lambda = [](int i) { return i < 0; };
+}
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  int np_local0 = 42;
+
+  {
+int p_local1 = 42;
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared 'const'
+np_local0 *= 2;
+  }
+}
+
+void ignore_reference_to_pointers() {
+  int *np_local0 = nullptr;
+  int *_local1 = np_local0;
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = _local0;
+  int *const p1_np_local0 = _local0;
+
+  int np_local1 = 42;
+  const int *const p0_np_local1 = _local1;
+  int *const p1_np_local1 = _local1;
+  *p1_np_local0 = 43;
+
+  int np_local2 = 42;
+  

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

[Jonas Toth via Phabricator via cfe-commits]

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

- write documentation for const-analysis check


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/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  clang-tools-extra/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,968 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
+// 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
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+// Lambdas should be ignored, because they do not follow the normal variable
+// semantic (e.g. the type is only known to the compiler).
+void lambdas() {
+  auto Lambda = [](int i) { return i < 0; };
+}
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  int np_local0 = 42;
+
+  {
+int p_local1 = 42;
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared 'const'
+np_local0 *= 2;
+  }
+}
+
+void ignore_reference_to_pointers() {
+  int *np_local0 = nullptr;
+  int *_local1 = np_local0;
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = _local0;
+  int *const p1_np_local0 = _local0;
+
+  int np_local1 = 42;
+  const int *const p0_np_local1 = _local1;
+  int *const p1_np_local1 = _local1;
+  *p1_np_local0 = 43;
+
+  int np_local2 = 42;
+  function_inout_pointer(_local2);
+
+  // Prevents const.
+  int np_local3 = 42;
+  out = _local3; // This returns and invalid address, its just about the AST
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' 

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

[Jonas Toth via Phabricator via cfe-commits]

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

- Merge branch 'main' into feature_rebase_const_transform_20210808


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/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,968 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
+// 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
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+// Lambdas should be ignored, because they do not follow the normal variable
+// semantic (e.g. the type is only known to the compiler).
+void lambdas() {
+  auto Lambda = [](int i) { return i < 0; };
+}
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  int np_local0 = 42;
+
+  {
+int p_local1 = 42;
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared 'const'
+np_local0 *= 2;
+  }
+}
+
+void ignore_reference_to_pointers() {
+  int *np_local0 = nullptr;
+  int *_local1 = np_local0;
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = _local0;
+  int *const p1_np_local0 = _local0;
+
+  int np_local1 = 42;
+  const int *const p0_np_local1 = _local1;
+  int *const p1_np_local1 = _local1;
+  *p1_np_local0 = 43;
+
+  int np_local2 = 42;
+  function_inout_pointer(_local2);
+
+  // Prevents const.
+  int np_local3 = 42;
+  out = _local3; // This returns and invalid address, its just about the AST
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D54943#2944680 , @0x8000- wrote:

> Using the checker now in our production BuildBot - no crashes and no false 
> positives. Can't say if there are false negatives, but at any rate the 
> checker is better than most colleagues at finding what should be declared 
> const.

Perfect, that sounds very good!
I will now continue with a bit of testing on open-source software, and if there 
are no crashes left, I would like to land this version.

`transformation` can then be used to iron out the false positives and have this 
check stabilize even further. But if it is useful (especially for linting 
during dev), there is value :)


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: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Whats left from my personal todo is adjusting the documentation.
I think then this check can work as linter and if you want as fixer as well, 
but this has to be enable explicitly.

I think that fixing still has more value than harm, e.g. in your IDE/editor. 
The most annyoing part is that `const` may be applied multiple times. But you 
can delete the superfluous consts and then you are done,  the variable will not 
be considered anymore.

Did I miss important things?




Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:46
+
+void ConstCorrectnessCheck::registerMatchers(MatchFinder *Finder) {
+  const auto ConstType = hasType(isConstQualified());

aaron.ballman wrote:
> JonasToth wrote:
> > aaron.ballman wrote:
> > > JonasToth wrote:
> > > > aaron.ballman wrote:
> > > > > Should this check only fire in C++? I'm sort of on the fence. It's a 
> > > > > C++ core guideline, so it stands to reason it should be disabled for 
> > > > > C code. But const-correctness is a thing in C too. WDYT?
> > > > I do not know all the subtle differences between C and C++ here. 
> > > > Judging from this: 
> > > > https://stackoverflow.com/questions/5248571/is-there-const-in-c the 
> > > > current form would not be correct for C for pointers.
> > > > The assumptions of this check and especially the transformations are 
> > > > done for C++. I dont see a reason why the value-semantic would not 
> > > > apply for both.
> > > > 
> > > > Maybe there is a way to make the code compatible for both languages. 
> > > > The easiest solution would probably to not do the 'pointer-as-value' 
> > > > transformation. This is not that relevant as a feature anyway. I expect 
> > > > not nearly as much usage of this option as for the others.
> > > > 
> > > > In the end of the day i would like to support both C and C++. Right now 
> > > > it is untested and especially the transformation might break the code. 
> > > > It should run on both languages though, as there is no language 
> > > > checking.
> > > > I will add some real world C code-bases for the transformation testing 
> > > > and see what happens :)
> > > > Judging from this: 
> > > > https://stackoverflow.com/questions/5248571/is-there-const-in-c the 
> > > > current form would not be correct for C for pointers.
> > > 
> > > Sure, there may be additional changes needed to support the other 
> > > oddities of C. I was asking more at the high level.
> > > 
> > > > In the end of the day i would like to support both C and C++. Right now 
> > > > it is untested and especially the transformation might break the code. 
> > > 
> > > Another option is for us to restrict the check in its current form to 
> > > just C++ and then expose a C check from it in a follow-up (likely under a 
> > > different module than cppcodeguidelines). For instance, CERT has a 
> > > recommendation (not a rule) about this for C 
> > > (https://wiki.sei.cmu.edu/confluence/display/c/DCL00-C.+Const-qualify+immutable+objects)
> > >  and I would not be surprised to find it in other coding standards as 
> > > well.
> > Another const check is probably better. The clang-tidy part should be 
> > minimal effort. I think there isn't alot of duplication either.
> > We could still think of proper configurability of this check and alias it 
> > into other modules with proper defaults for C.
> Okay, I'm sold -- can you not register the check unless in C++ mode? We can 
> add a C-specific check later.
Register only for C++



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h:31
+WarnPointersAsValues(Options.get("WarnPointersAsValues", 0)),
+TransformValues(Options.get("TransformValues", 1)),
+TransformReferences(Options.get("TransformReferences", 1)),

Deactivating the transformations by default, as they are not ready for 
production yet.



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:830
+
+void complex_usage() {
+  int np_local0 = 42;

aaron.ballman wrote:
> Here's another terrible one to try -- can you make sure we don't try to make 
> something const when the variable is evaluated in a context where it's 
> usually not:
> ```
> int N = 1; // Can't make N 'const' because VLAs make everything awful
> sizeof(int[++N]);
> ```
Already covered ;) I was afraid :D


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: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 365257.
JonasToth marked 4 inline comments as done.
JonasToth added a comment.

- remove transformation as default option, now it must be activated by the user 
explicitly
- register only for C++
- add a test-case for VLAs


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/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,968 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
+// 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
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+// Lambdas should be ignored, because they do not follow the normal variable
+// semantic (e.g. the type is only known to the compiler).
+void lambdas() {
+  auto Lambda = [](int i) { return i < 0; };
+}
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  int np_local0 = 42;
+
+  {
+int p_local1 = 42;
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared 'const'
+np_local0 *= 2;
+  }
+}
+
+void ignore_reference_to_pointers() {
+  int *np_local0 = nullptr;
+  int *_local1 = np_local0;
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = _local0;
+  int *const p1_np_local0 = _local0;
+
+  int np_local1 = 42;
+  const int *const p0_np_local1 = _local1;
+  int *const p1_np_local1 = _local1;
+  *p1_np_local0 = 43;
+
+  int np_local2 = 42;
+  function_inout_pointer(_local2);
+
+  // Prevents const.
+  int np_local3 = 42;
+  out = _local3; // This returns and invalid address, its just about the AST
+
+ 

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

[Jonas Toth via Phabricator via cfe-commits]

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

- retry upload of patch to be a diff to main


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/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,963 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
+// 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
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+// Lambdas should be ignored, because they do not follow the normal variable
+// semantic (e.g. the type is only known to the compiler).
+void lambdas() {
+  auto Lambda = [](int i) { return i < 0; };
+}
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  int np_local0 = 42;
+
+  {
+int p_local1 = 42;
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared 'const'
+np_local0 *= 2;
+  }
+}
+
+void ignore_reference_to_pointers() {
+  int *np_local0 = nullptr;
+  int *_local1 = np_local0;
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = _local0;
+  int *const p1_np_local0 = _local0;
+
+  int np_local1 = 42;
+  const int *const p0_np_local1 = _local1;
+  int *const p1_np_local1 = _local1;
+  *p1_np_local0 = 43;
+
+  int np_local2 = 42;
+  function_inout_pointer(_local2);
+
+  // Prevents const.
+  int np_local3 = 42;
+  out = _local3; // This returns and invalid address, its just about the AST
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+  const int 

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 365252.
JonasToth marked an inline comment as done.
JonasToth added a comment.

- rebase to master
- fix a crash where a scope matched but non of its variables, leading to 
nullptr dereference (found with the current test-suite)
- remove outcommenting of tests that showed undesirable behavior


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/cppcoreguidelines/ConstCorrectnessCheck.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp


Index: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
===
--- 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
+++ 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -920,7 +920,6 @@
   my_function_type ptr = function_ref_target;
 }
 
-#if 0
 template 
 T _ref() {
   static T global;
@@ -935,10 +934,16 @@
   // for the analysis. That results in bad transformations.
   auto auto_val0 = T{};
   auto _val1 = auto_val0; // Bad
+  // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'auto_val1' of type 
'System &' can be declared 'const'
+  // CHECK-MESSAGES:[[@LINE-2]]:3: warning: variable 'auto_val1' of type 'int 
&' can be declared 'const'
   auto *auto_val2 = _val0;
 
-  auto auto_ref0 = return_ref();  // Bad
+  auto auto_ref0 = return_ref();
+  // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'auto_ref0' of type 
'System' can be declared 'const'
+  // CHECK-MESSAGES:[[@LINE-2]]:3: warning: variable 'auto_ref0' of type 'int' 
can be declared 'const'
   auto _ref1 = return_ref(); // Bad
+  // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'auto_ref1' of type 
'System &' can be declared 'const'
+  // CHECK-MESSAGES:[[@LINE-2]]:3: warning: variable 'auto_ref1' of type 'int 
&' can be declared 'const'
   auto *auto_ref2 = return_ptr();
 
   auto auto_ptr0 = return_ptr();
@@ -948,10 +953,11 @@
   using MyTypedef = T;
   auto auto_td0 = MyTypedef{};
   auto _td1 = auto_td0; // Bad
+  // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'auto_td1' of type 
'System &' can be declared 'const'
+  // CHECK-MESSAGES:[[@LINE-2]]:3: warning: variable 'auto_td1' of type 'int 
&' can be declared 'const'
   auto *auto_td2 = _td0;
 }
 void instantiate_auto_cases() {
   auto_usage_variants();
   auto_usage_variants();
 }
-#endif
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
===
--- clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
@@ -90,11 +90,12 @@
 
 void ConstCorrectnessCheck::check(const MatchFinder::MatchResult ) {
   const auto *LocalScope = Result.Nodes.getNodeAs("scope");
-  assert(LocalScope && "Did not match scope for local variable");
-  registerScope(LocalScope, Result.Context);
-
   const auto *Variable = Result.Nodes.getNodeAs("local-value");
-  assert(Variable && "Did not match local variable definition");
+
+  // There have been crashes on 'Variable == nullptr', even though the matcher
+  // is not conditional. This comes probably from 'findAll'-matching.
+  if (!Variable || !LocalScope)
+return;
 
   VariableCategory VC = VariableCategory::Value;
   if (Variable->getType()->isReferenceType())
@@ -118,6 +119,9 @@
   if (VC == VariableCategory::Value && !AnalyzeValues)
 return;
 
+  // The scope is only registered if the analysis shall be run.
+  registerScope(LocalScope, Result.Context);
+
   // Offload const-analysis to utility function.
   if (ScopesCache[LocalScope]->isMutated(Variable))
 return;


Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
===
--- clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -920,7 +920,6 @@
   my_function_type ptr = function_ref_target;
 }
 
-#if 0
 template 
 T _ref() {
   static T global;
@@ -935,10 +934,16 @@
   // for the analysis. That results in bad transformations.
   auto auto_val0 = T{};
   auto _val1 = auto_val0; // Bad
+  // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'auto_val1' of type 'System &' can be declared 'const'
+  // CHECK-MESSAGES:[[@LINE-2]]:3: warning: variable 'auto_val1' of type 'int &' can be declared 'const'
   auto *auto_val2 = _val0;
 
-  auto auto_ref0 = return_ref();  // Bad
+  auto auto_ref0 = return_ref();
+  // CHECK-MESSAGES:[[@LINE-1]]:3: warning: variable 'auto_ref0' of type 'System' can be declared 'const'
+  // CHECK-MESSAGES:[[@LINE-2]]:3: warning: 

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

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D54943#2633408 , @tiagoma wrote:

> Can we get this in? I work in Microsoft Office and we have been using this 
> checker and it works great! There are a couple of issues with it and I would 
> like to contribute fixes.

Hey,

YES. I WILL WORK ON THIS NOW.

After s much time has passed I think i have finally time again to bring 
this check over the line. I will invest at least every sunday from now on, 
until its done :)

This is my initial work-list I would like to fix before this check can be 
merged:

- rebase to current master (obviously)
- fix `clang-apply-replacements` duplication that comes from fixes in templates 
(multiple instantiations create `const`-fix at the same position. but because 
the warning message contains different type names,  they are not deduplicated)
- go through the review again and check if there are missing comments to address
- improve the documentation and give some hints on possible issues

I think from this point on the check is ready to be improved, as there will be 
only false positive/false negatives left.

What I observed during the initial development time was, that llvm's orcjit 
tests failed (i believe with a crash) after a full-llvm-transformation.
This is most likely UB from casting/`std::launder`or so? Given the interest in 
general for this checker, we should provide some warnings that this can happen 
and maybe figure out what the issues is (I failed so far).


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] D90972: [clang-tidy] Install run-clang-tidy.py in bin/ as run-clang-tidy

[Jonas Toth via Phabricator via cfe-commits]

JonasToth accepted this revision.
JonasToth added a comment.

LGTM! thanks for fixing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90972

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

[PATCH] D89194: [clang-tidy] Fix crash in readability-function-cognitive-complexity on weak refs

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

You can land it. Worst case would be post-commit comments, but this is not a 
complicated patch and should be fine!


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

https://reviews.llvm.org/D89194

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

[PATCH] D89194: [clang-tidy] Fix crash in readability-function-cognitive-complexity on weak refs

[Jonas Toth via Phabricator via cfe-commits]

JonasToth accepted this revision.
JonasToth added a comment.

I am fine now, thank you!


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

https://reviews.llvm.org/D89194

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

[PATCH] D79113: Revert "Remove false positive in AvoidNonConstGlobalVariables."

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

The issue is resolved now in the CppCoreGuidelines.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79113

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

[PATCH] D89194: [clang-tidy] Fix crash in readability-function-cognitive-complexity on weak refs

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

The context thingie was not that important here, but still thanks :)




Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4658
+/// functionDecl(isWeak())
+///   matches the weak declaration foo, but not bar.
+AST_MATCHER(FunctionDecl, isWeak) { return Node.isWeak(); }

zinovy.nis wrote:
> JonasToth wrote:
> > Short oversight. Please hightlight the code and matcher to make a 
> > differentation in the docs.
> Sorry, I did not fully get what you mean as I just cloned isDefaulted section.
Its not consistent in the docs, but e.g.
`typedefNameDecl() matches "typedef int X" and "using Y = int"` is just more 
readable in the slightly formatted docs.

my preference would be `"functionDecl(isWeak()) matches the weak declaration 
"foo", but not "bar".`
(If you do the chance don't forget to recreate the HTML)


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

https://reviews.llvm.org/D89194

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

[PATCH] D89194: [clang-tidy] Fix crash in readability-function-cognitive-complexity on weak refs

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang/include/clang/ASTMatchers/ASTMatchers.h:4658
+/// functionDecl(isWeak())
+///   matches the weak declaration foo, but not bar.
+AST_MATCHER(FunctionDecl, isWeak) { return Node.isWeak(); }

Short oversight. Please hightlight the code and matcher to make a 
differentation in the docs.


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

https://reviews.llvm.org/D89194

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

[PATCH] D89194: [clang-tidy] Fix crash in readability-function-cognitive-complexity on weak refs

[Jonas Toth via Phabricator via cfe-commits]

JonasToth accepted this revision.
JonasToth added a comment.

LGTM.
Short reminder to please use full context patches. Its not an issue right now, 
but review sometimes just needs the context to understand the changes :)


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

https://reviews.llvm.org/D89194

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

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

[Jonas Toth via Phabricator via cfe-commits]

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

- fix test RUN line


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/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,957 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
+// 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
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+// Lambdas should be ignored, because they do not follow the normal variable
+// semantic (e.g. the type is only known to the compiler).
+void lambdas() {
+  auto Lambda = [](int i) { return i < 0; };
+}
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  int np_local0 = 42;
+
+  {
+int p_local1 = 42;
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared 'const'
+np_local0 *= 2;
+  }
+}
+
+void ignore_reference_to_pointers() {
+  int *np_local0 = nullptr;
+  int *_local1 = np_local0;
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = _local0;
+  int *const p1_np_local0 = _local0;
+
+  int np_local1 = 42;
+  const int *const p0_np_local1 = _local1;
+  int *const p1_np_local1 = _local1;
+  *p1_np_local0 = 43;
+
+  int np_local2 = 42;
+  function_inout_pointer(_local2);
+
+  // Prevents const.
+  int np_local3 = 42;
+  out = _local3; // This returns and invalid address, its just about the AST
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+  const int *const p0_p_local1 = _local1;

[PATCH] D54943: WIP [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 inline comments.



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:46
+
+void ConstCorrectnessCheck::registerMatchers(MatchFinder *Finder) {
+  const auto ConstType = hasType(isConstQualified());

aaron.ballman wrote:
> JonasToth wrote:
> > aaron.ballman wrote:
> > > Should this check only fire in C++? I'm sort of on the fence. It's a C++ 
> > > core guideline, so it stands to reason it should be disabled for C code. 
> > > But const-correctness is a thing in C too. WDYT?
> > I do not know all the subtle differences between C and C++ here. Judging 
> > from this: https://stackoverflow.com/questions/5248571/is-there-const-in-c 
> > the current form would not be correct for C for pointers.
> > The assumptions of this check and especially the transformations are done 
> > for C++. I dont see a reason why the value-semantic would not apply for 
> > both.
> > 
> > Maybe there is a way to make the code compatible for both languages. The 
> > easiest solution would probably to not do the 'pointer-as-value' 
> > transformation. This is not that relevant as a feature anyway. I expect not 
> > nearly as much usage of this option as for the others.
> > 
> > In the end of the day i would like to support both C and C++. Right now it 
> > is untested and especially the transformation might break the code. It 
> > should run on both languages though, as there is no language checking.
> > I will add some real world C code-bases for the transformation testing and 
> > see what happens :)
> > Judging from this: 
> > https://stackoverflow.com/questions/5248571/is-there-const-in-c the current 
> > form would not be correct for C for pointers.
> 
> Sure, there may be additional changes needed to support the other oddities of 
> C. I was asking more at the high level.
> 
> > In the end of the day i would like to support both C and C++. Right now it 
> > is untested and especially the transformation might break the code. 
> 
> Another option is for us to restrict the check in its current form to just 
> C++ and then expose a C check from it in a follow-up (likely under a 
> different module than cppcodeguidelines). For instance, CERT has a 
> recommendation (not a rule) about this for C 
> (https://wiki.sei.cmu.edu/confluence/display/c/DCL00-C.+Const-qualify+immutable+objects)
>  and I would not be surprised to find it in other coding standards as well.
Another const check is probably better. The clang-tidy part should be minimal 
effort. I think there isn't alot of duplication either.
We could still think of proper configurability of this check and alias it into 
other modules with proper defaults for C.


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: WIP [clang-tidy] implement const-transformation for cppcoreguidelines-const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- no delayed template parsing
- adjust release note issue


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/cppcoreguidelines/CMakeLists.txt
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp
  clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.h
  clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tools-extra/docs/ReleaseNotes.rst
  
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines-const-correctness.rst
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-cxx17.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-pointer-as-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-transform-values.cpp
  
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp

Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp
@@ -0,0 +1,957 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const-correctness %t -- \
+// RUN:   -config="{CheckOptions: [\
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformValues', value: 1}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.WarnPointersAsValues', value: 0}, \
+// RUN:   {key: 'cppcoreguidelines-const-correctness.TransformPointersAsValues', value: 0}, \
+// 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
+
+namespace foo {
+int scoped;
+float np_scoped = 1; // namespace variables are like globals
+} // namespace foo
+
+// Lambdas should be ignored, because they do not follow the normal variable
+// semantic (e.g. the type is only known to the compiler).
+void lambdas() {
+  auto Lambda = [](int i) { return i < 0; };
+}
+
+void some_function(double, wchar_t);
+
+void some_function(double np_arg0, wchar_t np_arg1) {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local0;
+  const int np_local1 = 42;
+
+  unsigned int np_local2 = 3;
+  np_local2 <<= 4;
+
+  int np_local3 = 4;
+  ++np_local3;
+  int np_local4 = 4;
+  np_local4++;
+
+  int np_local5 = 4;
+  --np_local5;
+  int np_local6 = 4;
+  np_local6--;
+}
+
+void nested_scopes() {
+  int p_local0 = 2;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+  int np_local0 = 42;
+
+  {
+int p_local1 = 42;
+// CHECK-MESSAGES: [[@LINE-1]]:5: warning: variable 'p_local1' of type 'int' can be declared 'const'
+np_local0 *= 2;
+  }
+}
+
+void ignore_reference_to_pointers() {
+  int *np_local0 = nullptr;
+  int *_local1 = np_local0;
+}
+
+void some_lambda_environment_capture_all_by_reference(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local2;
+  const int np_local3 = 2;
+
+  // Capturing all variables by reference prohibits making them const.
+  [&]() { ++np_local0; };
+
+  int p_local1 = 0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+}
+
+void some_lambda_environment_capture_all_by_value(double np_arg0) {
+  int np_local0 = 0;
+  int p_local0 = 1;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared 'const'
+
+  int np_local1;
+  const int np_local2 = 2;
+
+  // Capturing by value has no influence on them.
+  [=]() { (void)p_local0; };
+
+  np_local0 += 10;
+}
+
+void function_inout_pointer(int *inout);
+void function_in_pointer(const int *in);
+
+void some_pointer_taking(int *out) {
+  int np_local0 = 42;
+  const int *const p0_np_local0 = _local0;
+  int *const p1_np_local0 = _local0;
+
+  int np_local1 = 42;
+  const int *const p0_np_local1 = _local1;
+  int *const p1_np_local1 = _local1;
+  *p1_np_local0 = 43;
+
+  int np_local2 = 42;
+  function_inout_pointer(_local2);
+
+  // Prevents const.
+  int np_local3 = 42;
+  out = _local3; // This returns and invalid address, its just about the AST
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared 'const'
+