[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

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

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.


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

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#2933179 , @JonasToth wrote:

> 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.
>
> YES. I WILL WORK ON THIS NOW.

Great news Jonas! Thank you!

> 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

These all seem to handle the fix-it portion. Would it be possible to split that 
into a follow-up review?

I am using tidy from CI and as a 'live-linter' from [n]vim. If I get a warning, 
it's easy to go to the line and type the six characters. In fact I don't even 
know the keystroke sequence that would instruct the editor to apply a fix-it :)

Obviously fix-its are not helping in CI, either - what is important is 
reporting at least one location and then the review gets rejected and 
subsequently revised.

I'm not saying I won't ever use the fix-its, but for now detection and 
reporting cover 100% of my use cases.

Thank you again for getting the energy and the time to work on this!


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.

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

[Tiago Macarios via Phabricator via cfe-commits]

tiagoma added a comment.
Herald added a subscriber: shchenz.

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.


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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D54943#2352196 , @0x8000- wrote:

> Would it be possible to split this review further, into "checking" and 
> "fixing" portions? I understand that fix-it portion is more difficult, and 
> sometimes results in multiple 'const' keywords being added, but for the past 
> year we have used the check as part of regular CI runs to flag where it needs 
> to be added by the engineers and for our use case it works fine that way - so 
> I would prefer to have at least the "report" part as part of upstream 
> Clang12, even if the 'fixup' comes later, due to the complexity of covering 
> all corner cases.

I wouldn't have an issue with that.


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

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

Would it be possible to split this review further, into "checking" and "fixing" 
portions? I understand that fix-it portion is more difficult, and sometimes 
results in multiple 'const' keywords being added, but for the past year we have 
used the check as part of regular CI runs to flag where it needs to be added by 
the engineers and for our use case it works fine that way - so I would prefer 
to have at least the "report" part as part of upstream Clang12, even if the 
'fixup' comes later, due to the complexity of covering all corner cases.


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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



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

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.



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

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]);
```



Comment at: 
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-const-correctness-values.cpp:923
+
+#if 0
+template 

Rather than comment out the code, I think it makes more sense to leave the code 
here, CHECK for the existing diagnostics, but add a FIXME comment to any 
diagnostics (or lack of diagnostics) that are incorrect. It makes it more clear 
what the current behavior is and what we want it to eventually be.


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 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'
+ 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



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

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.



Comment at: clang-tools-extra/docs/ReleaseNotes.rst:121
+^
+>>> 8d940dfbccb... remove spurious formatting change
+

This looks unintentional to me.


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 297231.
JonasToth added a comment.

- missed one place of decltype


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:   ]}" --
+
+// --- 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;
+
+  int p_local2 

[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 297224.
JonasToth added a comment.

- fix platform dependent warning message for decltype to just match the 
beginning and not the 'a.k.a'


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:   ]}" --
+
+// --- 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 

[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 297215.
JonasToth marked an inline comment as done.
JonasToth added a comment.

- address 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/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:   ]}" --
+
+// --- 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 

[PATCH] D54943: WIP [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/clang-tidy/cppcoreguidelines/CMakeLists.txt:37
+  clangLex
+  clangSerialization
   clangTidy

aaron.ballman wrote:
> Why do serialization and lex need to be pulled in?
Thats a good question 樂 
Probably old artifact from where everything was done here.
Removed everything except analysis



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

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 :)


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 297197.
JonasToth added a comment.

- update to landed ExprMutAnalyzer 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/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:   ]}" --
+
+// --- 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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt:37
+  clangLex
+  clangSerialization
   clangTidy

Why do serialization and lex need to be pulled in?



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

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?



Comment at: 
clang-tools-extra/clang-tidy/cppcoreguidelines/ConstCorrectnessCheck.cpp:105
+
+  // Each variable can only in one category: Value, Pointer, Reference.
+  // Analysis can be controlled for every category.

can only in -> can only be in


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.

In D54943#2302315 , @0x8000- wrote:

> In D54943#2292377 , @0x8000- 
> wrote:
>
>> In D54943#2291969 , @JonasToth 
>> wrote:
>>
>>> @AlexanderLanin @0x8000- i created the branch `release-11-const` 
>>> (https://github.com/JonasToth/llvm-project/tree/release-11-const) in my 
>>> fork.
>>>
>>> This branch is based on 11-rc3 and cherry picks the commits that correspond 
>>> to this revision.
>>> I hope this is of any use for you! I will probably forget to update this 
>>> branch all the time, but if I feel like there has been actual progress 
>>> made, i will update!
>>
>> Thank you Jonas - will add this to our CI tool for the new project and try 
>> to find some time to run it on the legacy project.
>
> Just a quick update; in clang-tidy mode it works great, with no false 
> positive and no crashes. I haven't tried the 'apply-replacements' mode.

That is great to hear! I update the `release-11-const` branch with the current 
state.

If you try the replacement-mode (with `run-clang-tidy`) beware, that there is a 
bug in the deduplication of clang-apply-replacements and some templated 
functions result in multiple applications of `const`.
This is something i need to work on, before everything can land :)


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 296549.
JonasToth added a comment.

rebase to newest expmutanalyzer patch


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
  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
@@ -19,9 +19,7 @@
 
 using namespace clang::ast_matchers;
 using ::testing::ElementsAre;
-using ::testing::IsEmpty;
 using ::testing::ResultOf;
-using ::testing::StartsWith;
 using ::testing::Values;
 
 namespace {
@@ -63,12 +61,16 @@
   const auto *const S = selectFirst("stmt", Results);
   SmallVector Chain;
   ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
+
   for (const auto *E = selectFirst("expr", Results); E != nullptr;) {
 const Stmt *By = Analyzer.findMutation(E);
-std::string buffer;
-llvm::raw_string_ostream stream(buffer);
-By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
-Chain.push_back(StringRef(stream.str()).trim().str());
+if (!By)
+  break;
+
+std::string Buffer;
+llvm::raw_string_ostream Stream(Buffer);
+By->printPretty(Stream, nullptr, AST->getASTContext().getPrintingPolicy());
+Chain.emplace_back(StringRef(Stream.str()).trim().str());
 E = dyn_cast(By);
   }
   return Chain;
@@ -111,7 +113,13 @@
 
 class AssignmentTest : public ::testing::TestWithParam {};
 
+// This test is for the most basic and direct modification of a variable,
+// assignment to it (e.g. `x = 10;`).
+// It additionally tests, that reference to a variable are not only captured
+// directly, but expression that result in the variable are handled, too.
+// This includes the comma operator, parens and the ternary operator.
 TEST_P(AssignmentTest, AssignmentModifies) {
+  // Test the detection of the raw expression modifications.
   {
 const std::string ModExpr = "x " + GetParam() + " 10";
 const auto AST = buildASTFromCode("void f() { int x; " + ModExpr + "; }");
@@ -120,6 +128,7 @@
 EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
   }
 
+  // Test the detection if the expression is surrounded by parens.
   {
 const std::string ModExpr = "(x) " + GetParam() + " 10";
 const auto AST = buildASTFromCode("void f() { int x; " + ModExpr + "; }");
@@ -127,12 +136,79 @@
 match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
 EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
   }
+
+  // Test the detection if the comma operator yields the expression as result.
+  {
+const std::string ModExpr = "x " + GetParam() + " 10";
+const auto AST = buildASTFromCodeWithArgs(
+"void f() { int x, y; y, " + ModExpr + "; }", {"-Wno-unused-value"});
+const auto Results =
+match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+  }
+
+  // Ensure no detection if t he comma operator does not yield the expression as
+  // result.
+  {
+const std::string ModExpr = "y, x, y " + GetParam() + " 10";
+const auto AST = buildASTFromCodeWithArgs(
+"void f() { int x, y; " + ModExpr + "; }", {"-Wno-unused-value"});
+const auto Results =
+match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+EXPECT_FALSE(isMutated(Results, AST.get()));
+  }
+
+  // Test the detection if the a ternary operator can result in the expression.
+  {
+const std::string ModExpr = "(y != 0 ? y : x) " + GetParam() + " 10";
+const auto AST =
+buildASTFromCode("void f() { int y = 0, x; " + ModExpr + "; }");
+const auto Results =
+match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+EXPECT_THAT(mutatedBy(Results, 

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

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#2292377 , @0x8000- wrote:

> In D54943#2291969 , @JonasToth wrote:
>
>> @AlexanderLanin @0x8000- i created the branch `release-11-const` 
>> (https://github.com/JonasToth/llvm-project/tree/release-11-const) in my fork.
>>
>> This branch is based on 11-rc3 and cherry picks the commits that correspond 
>> to this revision.
>> I hope this is of any use for you! I will probably forget to update this 
>> branch all the time, but if I feel like there has been actual progress made, 
>> i will update!
>
> Thank you Jonas - will add this to our CI tool for the new project and try to 
> find some time to run it on the legacy project.

Just a quick update; in clang-tidy mode it works great, with no false positive 
and no crashes. I haven't tried the 'apply-replacements' mode.


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.

In D54943#2295894 , @AlexanderLanin 
wrote:

> Off-Topic: I was just attempting to apply this to my codebase at work.
> Seems this will be more challenging than anticipated  
> My best guess is this is related to D72730 
>
>   Applying fixes ...
>   terminate called after throwing an instance of 'std::bad_alloc'
> what():  std::bad_alloc

Lets start with this one:
What did you exactly do?
Is this a run of `run-clang-tidy` for a whole codebase?
In the end, it could actually just boil down to "not enough ram", because 
`clang-apply-replacements` do merge the diagnostics and stuff at some point.
Could you please provide more information about your issue here so that it is 
possible to root out other issues than memory-limitations. Even those should be 
addressed in some way.


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

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

Off-Topic: I was just attempting to apply this to my codebase at work.
Seems this will be more challenging than anticipated  
My best guess is this is related to D72730 

  Applying fixes ...
  terminate called after throwing an instance of 'std::bad_alloc'
what():  std::bad_alloc


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

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

"Minimal" example for the behavior I described before.

`clang++ -fsyntax-only  ~/llvm/clang/test/SemaCXX/warn-range-loop-analysis.cpp 
-Wrange-loop-analysis 2>&1 | wc -l`
269

Then fix it by:
`~/llvm/build-const-fix/bin/clang-tidy 
~/llvm/clang/test/SemaCXX/warn-range-loop-analysis.cpp 
-checks="-*,cppcoreguidelines-const-correctness" -fix`

And recount:
`clang++ -fsyntax-only  ~/llvm/clang/test/SemaCXX/warn-range-loop-analysis.cpp 
-Wrange-loop-analysis 2>&1 | wc -l`
283

Yet again I want to mentioned that this is technically not a bug, but in 
combination with Werror it's not ideal. Basically I cannot compile my codebase 
after this fixit since Werror is set. Although the code is arguably better.


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

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#2291969 , @JonasToth wrote:

> @AlexanderLanin @0x8000- i created the branch `release-11-const` 
> (https://github.com/JonasToth/llvm-project/tree/release-11-const) in my fork.
>
> This branch is based on 11-rc3 and cherry picks the commits that correspond 
> to this revision.
> I hope this is of any use for you! I will probably forget to update this 
> branch all the time, but if I feel like there has been actual progress made, 
> i will update!

Thank you Jonas - will add this to our CI tool for the new project and try to 
find some time to run it on the legacy project.


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.

@AlexanderLanin @0x8000- i created the branch `release-11-const` 
(https://github.com/JonasToth/llvm-project/tree/release-11-const) in my fork.

This branch is based on 11-rc3 and cherry picks the commits that correspond to 
this revision.
I hope this is of any use for you! I will probably forget to update this branch 
all the time, but if I feel like there has been actual progress made, i will 
update!


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 293970.
JonasToth added a comment.

  rebase to current exprmutanalyzer
  
  - remove spurious formatting change
  - fix include and typo


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
  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
@@ -19,9 +19,7 @@
 
 using namespace clang::ast_matchers;
 using ::testing::ElementsAre;
-using ::testing::IsEmpty;
 using ::testing::ResultOf;
-using ::testing::StartsWith;
 using ::testing::Values;
 
 namespace {
@@ -63,13 +61,18 @@
   const auto *const S = selectFirst("stmt", Results);
   SmallVector Chain;
   ExprMutationAnalyzer Analyzer(*S, AST->getASTContext());
+
+  std::string buffer;
   for (const auto *E = selectFirst("expr", Results); E != nullptr;) {
 const Stmt *By = Analyzer.findMutation(E);
-std::string buffer;
+if (!By)
+  break;
+
 llvm::raw_string_ostream stream(buffer);
 By->printPretty(stream, nullptr, AST->getASTContext().getPrintingPolicy());
-Chain.push_back(StringRef(stream.str()).trim().str());
+Chain.emplace_back(StringRef(stream.str()).trim().str());
 E = dyn_cast(By);
+buffer.clear();
   }
   return Chain;
 }
@@ -111,7 +114,13 @@
 
 class AssignmentTest : public ::testing::TestWithParam {};
 
+// This test is for the most basic and direct modification of a variable,
+// assignment to it (e.g. `x = 10;`).
+// It additionally tests, that reference to a variable are not only captured
+// directly, but expression that result in the variable are handled, too.
+// This includes the comma operator, parens and the ternary operator.
 TEST_P(AssignmentTest, AssignmentModifies) {
+  // Test the detection of the raw expression modifications.
   {
 const std::string ModExpr = "x " + GetParam() + " 10";
 const auto AST = buildASTFromCode("void f() { int x; " + ModExpr + "; }");
@@ -120,6 +129,7 @@
 EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
   }
 
+  // Test the detection if the expression is surrounded by parens.
   {
 const std::string ModExpr = "(x) " + GetParam() + " 10";
 const auto AST = buildASTFromCode("void f() { int x; " + ModExpr + "; }");
@@ -127,6 +137,60 @@
 match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
 EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
   }
+
+  // Test the detection if the comma operator yields the expression as result.
+  {
+const std::string ModExpr = "x " + GetParam() + " 10";
+const auto AST = buildASTFromCodeWithArgs(
+"void f() { int x, y; y, " + ModExpr + "; }", {"-Wno-unused-value"});
+const auto Results =
+match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+  }
+
+  // Ensure no detection if t he comma operator does not yield the expression as
+  // result.
+  {
+const std::string ModExpr = "y, x, y " + GetParam() + " 10";
+const auto AST = buildASTFromCodeWithArgs(
+"void f() { int x, y; " + ModExpr + "; }", {"-Wno-unused-value"});
+const auto Results =
+match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+EXPECT_FALSE(isMutated(Results, AST.get()));
+  }
+
+  // Test the detection if the a ternary operator can result in the expression.
+  {
+const std::string ModExpr = "(y != 0 ? y : x) " + GetParam() + " 10";
+const auto AST =
+buildASTFromCode("void f() { int y = 0, x; " + ModExpr + "; }");
+const auto Results =
+match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+  }
+
+  

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

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#2290713 , @AlexanderLanin 
wrote:

>> Could you please provide me a full reproducer (optimally without 
>> dependencies on includes/libraries)?
>
> I can certainly do that based on my old version from ~9 months ago or 
> preferably if you provide me some branch somewhere (github?). I have trouble 
> applying the latest diff to latest master myself.

https://github.com/JonasToth/llvm-project/tree/feature_const_transform is close 
to the origin/master (a few commits behind).


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

[Alexander Lanin via Phabricator via cfe-commits]

AlexanderLanin added a comment.

> Could you please provide me a full reproducer (optimally without dependencies 
> on includes/libraries)?

I can certainly do that based on my old version from ~9 months ago or 
preferably if you provide me some branch somewhere (github?). I have trouble 
applying the latest diff to latest master myself.


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

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

In D54943#2289410 , @JonasToth wrote:

> In D54943#2289037 , @0x8000- 
> wrote:
>
>> Master branch has too many false positives for tidy - would it be possible 
>> to create a branch that contains this patch set on top of llvm-11.0-rc3? I 
>> would then add this to our internal CI.
>
> Yeah, it is not that simple to get this check into something really usefull :/

Just to be clear, the false positives I was referring to are not in this 
checker, but other checkers (uninitialized variables mostly) that were 
committed to the origin/main branch.

> I can create a branch in my own fork, that would need a little work, which i 
> can do in ~8 hours. I would then ping you.

Thank you - I tried cherry-picking the changes from the main branch onto the 
release branch and ... it does not build - didn't have time to debug it - and I 
am hoping you're more quick at making the adaptation.

>> For the legacy code (see previous reports) we found quite a few false 
>> positives. For a new code base that we're developing from scratch with 
>> C++20, there have been no false positives - it catches missed 'const' much 
>> better than human reviewers - we still keep a Clang10 around that I built 
>> from source with the previous version of the patches :)
>
> It would be great if you could keep testing your legacy code-base once this 
> patch progresses even further.
> It is a great message, that the modern codebase does not make big trouble :) 
> Given I tested mostly on LLVM itself so far, this is not too surprising as 
> its a modern codebase itself.

I will keep testing on both codebases - the modern one we're using it in the CI 
so it gets tested vigorously. Once I get some time I'll take another pass at 
the legacy code base to see what it finds.

Thank you for resuming the work on this. I was afraid I'm going to lose this 
check when we have to move off Clang10 - (we're pursuing C++20 aggressively in 
the new project).


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.

In D54943#2289037 , @0x8000- wrote:

> Master branch has too many false positives for tidy - would it be possible to 
> create a branch that contains this patch set on top of llvm-11.0-rc3? I would 
> then add this to our internal CI.

Yeah, it is not that simple to get this check into something really usefull :/
I can create a branch in my own fork, that would need a little work, which i 
can do in ~8 hours. I would then ping you.

> For the legacy code (see previous reports) we found quite a few false 
> positives. For a new code base that we're developing from scratch with C++20, 
> there have been no false positives - it catches missed 'const' much better 
> than human reviewers - we still keep a Clang10 around that I built from 
> source with the previous version of the patches :)

It would be great if you could keep testing your legacy code-base once this 
patch progresses even further.
It is a great message, that the modern codebase does not make big trouble :) 
Given I tested mostly on LLVM itself so far, this is not too surprising as its 
a modern codebase itself.


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

[Florin Iucha via Phabricator via cfe-commits]

0x8000- added a comment.

Master branch has too many false positives for tidy - would it be possible to 
create a branch that contains this patch set on top of llvm-11.0-rc3? I would 
then add this to our internal CI.

For the legacy code (see previous reports) we found quite a few false 
positives. For a new code base that we're developing from scratch with C++20, 
there have been no false positives - it catches missed 'const' much better than 
human reviewers - we still keep a Clang10 around that I built from source with 
the previous version of the patches :)


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