[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- rebase on commited ExpMutationAnalyzer
- clean up, documentation


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/ConstCheck.cpp
  clang-tidy/cppcoreguidelines/ConstCheck.h
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-const.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-const-pointers-as-values.cpp
  test/clang-tidy/cppcoreguidelines-const-values.cpp

Index: test/clang-tidy/cppcoreguidelines-const-values.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-const-values.cpp
@@ -0,0 +1,557 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const %t
+
+// --- 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
+
+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 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 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int' can be declared const
+  function_in_pointer(_local2);
+}
+
+void function_inout_ref(int );
+void function_in_ref(const int );
+
+void some_reference_taking() {
+  int np_local0 = 42;
+  const int _np_local0 = np_local0;
+  int _np_local0 = np_local0;
+  r1_np_local0 = 43;
+  const int _np_local0 = r1_np_local0;
+
+  int np_local1 = 42;
+  function_inout_ref(np_local1);
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int _p_local0 = p_local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_ref(p_local1);
+}
+
+double *non_const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double np_local0 = 24.4;
+
+  return _local0;
+}
+
+const double *const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double p_local1 = 24.4;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of 

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I see. Thank you for fixing + explaining :)

Am 30.04.2018 um 23:39 schrieb Shuai Wang via Phabricator:

> shuaiwang added a comment.
> 
>> Why do you think that looping is required? From my understanding, we
>> 
>>   need the first usage (DeclRefExpr) to get the analysis started. The
>>   analysis itself will check all remaining uses. Is this necessary,
>>   because we analysis on a `Expr` basis?
> 
> Yes. the analyzer traces starting from the given DeclRefExpr, e.g.:
> 
>   int a; // <- varDecl
>   const int& b = a;  // <- first DeclRefExpr
>   int& c = a; // <- second DeclRefExpr
>   
>   const int& b2 = b;
>   int& c2 = c;
>   c2 = 10;
> 
> If we start from the first DeclRefExpr, we'll only trace to `b` and then to 
> `b2`. We need to start from varDecl (or loop over all DeclRefExpr) to be able 
> to trace to the second DeclRefExpr from where we can get to `c` -> `c2` -> 
> `c2 = 10`.
> 
> Repository:
> 
>   rCTE Clang Tools Extra
> 
> https://reviews.llvm.org/D45444


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang added a comment.

> Why do you think that looping is required? From my understanding, we
>  need the first usage (DeclRefExpr) to get the analysis started. The
>  analysis itself will check all remaining uses. Is this necessary,
>  because we analysis on a `Expr` basis?

Yes. the analyzer traces starting from the given DeclRefExpr, e.g.:

  int a; // <- varDecl
  const int& b = a;  // <- first DeclRefExpr
  int& c = a; // <- second DeclRefExpr
  
  const int& b2 = b;
  int& c2 = c;
  c2 = 10;

If we start from the first DeclRefExpr, we'll only trace to `b` and then to 
`b2`. We need to start from varDecl (or loop over all DeclRefExpr) to be able 
to trace to the second DeclRefExpr from where we can get to `c` -> `c2` -> `c2 
= 10`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- fix bad template instantiation


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/ConstCheck.cpp
  clang-tidy/cppcoreguidelines/ConstCheck.h
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/utils/CMakeLists.txt
  clang-tidy/utils/ExprMutationAnalyzer.cpp
  clang-tidy/utils/ExprMutationAnalyzer.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-const.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-const-pointers-as-values.cpp
  test/clang-tidy/cppcoreguidelines-const-values.cpp
  unittests/clang-tidy/CMakeLists.txt
  unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

Index: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
@@ -0,0 +1,554 @@
+//===-- ExprMutationAnalyzerTest.cpp - clang-tidy -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "../clang-tidy/utils/ExprMutationAnalyzer.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+using namespace clang::ast_matchers;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::ResultOf;
+using ::testing::StartsWith;
+using ::testing::Values;
+
+namespace {
+
+using ExprMatcher = internal::Matcher;
+using StmtMatcher = internal::Matcher;
+
+ExprMatcher declRefTo(StringRef Name) {
+  return declRefExpr(to(namedDecl(hasName(Name;
+}
+
+StmtMatcher withEnclosingCompound(ExprMatcher Matcher) {
+  return expr(Matcher, hasAncestor(compoundStmt().bind("stmt"))).bind("expr");
+}
+
+bool isMutated(const SmallVectorImpl , ASTUnit *AST) {
+  const auto *const S = selectFirst("stmt", Results);
+  const auto *const E = selectFirst("expr", Results);
+  return utils::ExprMutationAnalyzer(S, >getASTContext()).isMutated(E);
+}
+
+SmallVector
+mutatedBy(const SmallVectorImpl , ASTUnit *AST) {
+  const auto *const S = selectFirst("stmt", Results);
+  SmallVector Chain;
+  utils::ExprMutationAnalyzer Analyzer(S, >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());
+E = dyn_cast(By);
+  }
+  return Chain;
+}
+
+std::string removeSpace(std::string s) {
+  s.erase(std::remove_if(s.begin(), s.end(),
+ [](char c) { return std::isspace(c); }),
+  s.end());
+  return s;
+}
+
+} // namespace
+
+TEST(ExprMutationAnalyzerTest, Trivial) {
+  const auto AST = tooling::buildASTFromCode("void f() { int x; x; }");
+  const auto Results =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+class AssignmentTest : public ::testing::TestWithParam {};
+
+TEST_P(AssignmentTest, AssignmentModifies) {
+  const std::string ModExpr = "x " + GetParam() + " 10";
+  const auto AST =
+  tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+  const auto Results =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllAssignmentOperators, AssignmentTest,
+Values("=", "+=", "-=", "*=", "/=", "%=", "&=", "|=",
+   "^=", "<<=", ">>="), );
+
+class IncDecTest : public ::testing::TestWithParam {};
+
+TEST_P(IncDecTest, IncDecModifies) {
+  const std::string ModExpr = GetParam();
+  const auto AST =
+  tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+  const auto Results =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllIncDecOperators, IncDecTest,
+Values("++x", "--x", "x++", "x--"), );
+
+TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
+  const auto AST = tooling::buildASTFromCode(
+  "void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
+  const auto Results =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- migrate to Decl interface
- add template metaprogramming test


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/ConstCheck.cpp
  clang-tidy/cppcoreguidelines/ConstCheck.h
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/utils/CMakeLists.txt
  clang-tidy/utils/ExprMutationAnalyzer.cpp
  clang-tidy/utils/ExprMutationAnalyzer.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-const.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-const-pointers-as-values.cpp
  test/clang-tidy/cppcoreguidelines-const-values.cpp
  unittests/clang-tidy/CMakeLists.txt
  unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

Index: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
@@ -0,0 +1,554 @@
+//===-- ExprMutationAnalyzerTest.cpp - clang-tidy -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "../clang-tidy/utils/ExprMutationAnalyzer.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+using namespace clang::ast_matchers;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::ResultOf;
+using ::testing::StartsWith;
+using ::testing::Values;
+
+namespace {
+
+using ExprMatcher = internal::Matcher;
+using StmtMatcher = internal::Matcher;
+
+ExprMatcher declRefTo(StringRef Name) {
+  return declRefExpr(to(namedDecl(hasName(Name;
+}
+
+StmtMatcher withEnclosingCompound(ExprMatcher Matcher) {
+  return expr(Matcher, hasAncestor(compoundStmt().bind("stmt"))).bind("expr");
+}
+
+bool isMutated(const SmallVectorImpl , ASTUnit *AST) {
+  const auto *const S = selectFirst("stmt", Results);
+  const auto *const E = selectFirst("expr", Results);
+  return utils::ExprMutationAnalyzer(S, >getASTContext()).isMutated(E);
+}
+
+SmallVector
+mutatedBy(const SmallVectorImpl , ASTUnit *AST) {
+  const auto *const S = selectFirst("stmt", Results);
+  SmallVector Chain;
+  utils::ExprMutationAnalyzer Analyzer(S, >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());
+E = dyn_cast(By);
+  }
+  return Chain;
+}
+
+std::string removeSpace(std::string s) {
+  s.erase(std::remove_if(s.begin(), s.end(),
+ [](char c) { return std::isspace(c); }),
+  s.end());
+  return s;
+}
+
+} // namespace
+
+TEST(ExprMutationAnalyzerTest, Trivial) {
+  const auto AST = tooling::buildASTFromCode("void f() { int x; x; }");
+  const auto Results =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+class AssignmentTest : public ::testing::TestWithParam {};
+
+TEST_P(AssignmentTest, AssignmentModifies) {
+  const std::string ModExpr = "x " + GetParam() + " 10";
+  const auto AST =
+  tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+  const auto Results =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllAssignmentOperators, AssignmentTest,
+Values("=", "+=", "-=", "*=", "/=", "%=", "&=", "|=",
+   "^=", "<<=", ">>="), );
+
+class IncDecTest : public ::testing::TestWithParam {};
+
+TEST_P(IncDecTest, IncDecModifies) {
+  const std::string ModExpr = GetParam();
+  const auto AST =
+  tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+  const auto Results =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllIncDecOperators, IncDecTest,
+Values("++x", "--x", "x++", "x--"), );
+
+TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
+  const auto AST = tooling::buildASTFromCode(
+  "void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
+  const auto Results =
+  match(withEnclosingCompound(declRefTo("x")), 

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I will migrate to the new API today evening (european time).

Why do you think that looping is required? From my understanding, we
need the first usage (DeclRefExpr) to get the analysis started. The
analysis itself will check all remaining uses. Is this necessary,
because we analysis on a `Expr` basis?

Am 29.04.2018 um 20:19 schrieb Shuai Wang via Phabricator:

> shuaiwang added inline comments.
> 
> 
>  Comment at: clang-tidy/cppcoreguidelines/ConstCheck.cpp:229-237
>  +  const auto *UseExpr = selectFirst("use", Usage);
>  +
>  +  // The declared variables was used in non-const conserving way and can not
>  +  // be declared as const.
>  +  if (UseExpr && Scopes[Scope]->isMutated(UseExpr)) {
>  +// diag(UseExpr->getLocStart(), "Investigating starting with this use",
>  +// DiagnosticIDs::Note);
> 
>  
> 
> I think we need to loop over usages instead of just checking the first, i.e.:
> 
>   for (const auto  : Usage) {
> const auto* UseExpr = Nodes.getNodeAs("use");
> if (UseExpr && isMutated(UseExpr)) return true;
>   }
> 
> 
> I'll add a helper function in the MutationAnalyzer for checking varDecl 
> directly as well per your comment there, which you'll be able to use directly 
> in this check. Before that's ready (and if you have time of course) could you 
> help check how many false positives are left with this change?
> 
> Repository:
> 
>   rCTE Clang Tools Extra
> 
> https://reviews.llvm.org/D45444


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang added inline comments.



Comment at: clang-tidy/cppcoreguidelines/ConstCheck.cpp:229-237
+  const auto *UseExpr = selectFirst("use", Usage);
+
+  // The declared variables was used in non-const conserving way and can not
+  // be declared as const.
+  if (UseExpr && Scopes[Scope]->isMutated(UseExpr)) {
+// diag(UseExpr->getLocStart(), "Investigating starting with this use",
+// DiagnosticIDs::Note);

I think we need to loop over usages instead of just checking the first, i.e.:
```
for (const auto  : Usage) {
  const auto* UseExpr = Nodes.getNodeAs("use");
  if (UseExpr && isMutated(UseExpr)) return true;
}
```

I'll add a helper function in the MutationAnalyzer for checking varDecl 
directly as well per your comment there, which you'll be able to use directly 
in this check. Before that's ready (and if you have time of course) could you 
help check how many false positives are left with this change?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- [Feature] use new statefull const checker
- [Test] add more tests finding false positives


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/ConstCheck.cpp
  clang-tidy/cppcoreguidelines/ConstCheck.h
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/utils/CMakeLists.txt
  clang-tidy/utils/ExprMutationAnalyzer.cpp
  clang-tidy/utils/ExprMutationAnalyzer.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-const.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-const-pointers-as-values.cpp
  test/clang-tidy/cppcoreguidelines-const-values.cpp
  unittests/clang-tidy/CMakeLists.txt
  unittests/clang-tidy/ExprMutationAnalyzerTest.cpp

Index: unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/ExprMutationAnalyzerTest.cpp
@@ -0,0 +1,528 @@
+//===-- ExprMutationAnalyzerTest.cpp - clang-tidy -===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "../clang-tidy/utils/ExprMutationAnalyzer.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+namespace test {
+
+using namespace clang::ast_matchers;
+using ::testing::ElementsAre;
+using ::testing::IsEmpty;
+using ::testing::ResultOf;
+using ::testing::StartsWith;
+using ::testing::Values;
+
+namespace {
+
+using ExprMatcher = internal::Matcher;
+using StmtMatcher = internal::Matcher;
+
+ExprMatcher declRefTo(StringRef Name) {
+  return declRefExpr(to(namedDecl(hasName(Name;
+}
+
+StmtMatcher withEnclosingCompound(ExprMatcher Matcher) {
+  return expr(Matcher, hasAncestor(compoundStmt().bind("stmt"))).bind("expr");
+}
+
+bool isMutated(const SmallVectorImpl , ASTUnit *AST) {
+  const auto *const S = selectFirst("stmt", Results);
+  const auto *const E = selectFirst("expr", Results);
+  return utils::ExprMutationAnalyzer(S, >getASTContext()).isMutated(E);
+}
+
+SmallVector
+mutatedBy(const SmallVectorImpl , ASTUnit *AST) {
+  const auto *const S = selectFirst("stmt", Results);
+  SmallVector Chain;
+  utils::ExprMutationAnalyzer Analyzer(S, >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());
+E = dyn_cast(By);
+  }
+  return Chain;
+}
+
+std::string removeSpace(std::string s) {
+  s.erase(std::remove_if(s.begin(), s.end(),
+ [](char c) { return std::isspace(c); }),
+  s.end());
+  return s;
+}
+
+} // namespace
+
+TEST(ExprMutationAnalyzerTest, Trivial) {
+  const auto AST = tooling::buildASTFromCode("void f() { int x; x; }");
+  const auto Results =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_FALSE(isMutated(Results, AST.get()));
+}
+
+class AssignmentTest : public ::testing::TestWithParam {};
+
+TEST_P(AssignmentTest, AssignmentModifies) {
+  const std::string ModExpr = "x " + GetParam() + " 10";
+  const auto AST =
+  tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+  const auto Results =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllAssignmentOperators, AssignmentTest,
+Values("=", "+=", "-=", "*=", "/=", "%=", "&=", "|=",
+   "^=", "<<=", ">>="), );
+
+class IncDecTest : public ::testing::TestWithParam {};
+
+TEST_P(IncDecTest, IncDecModifies) {
+  const std::string ModExpr = GetParam();
+  const auto AST =
+  tooling::buildASTFromCode("void f() { int x; " + ModExpr + "; }");
+  const auto Results =
+  match(withEnclosingCompound(declRefTo("x")), AST->getASTContext());
+  EXPECT_THAT(mutatedBy(Results, AST.get()), ElementsAre(ModExpr));
+}
+
+INSTANTIATE_TEST_CASE_P(AllIncDecOperators, IncDecTest,
+Values("++x", "--x", "x++", "x--"), );
+
+TEST(ExprMutationAnalyzerTest, NonConstMemberFunc) {
+  const auto AST = tooling::buildASTFromCode(
+  "void f() { struct Foo { void mf(); }; Foo x; x.mf(); }");
+  const auto Results =
+  

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- [Misc] mark false positives


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/ConstCheck.cpp
  clang-tidy/cppcoreguidelines/ConstCheck.h
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-const.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-const-handles.cpp
  test/clang-tidy/cppcoreguidelines-const-values.cpp

Index: test/clang-tidy/cppcoreguidelines-const-values.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-const-values.cpp
@@ -0,0 +1,433 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: "cppcoreguidelines-const.AnalyzeValues", value: 1},\
+// RUN:   {key: "cppcoreguidelines-const.AnalyzeHandles", value: 0},\
+// RUN:   {key: "cppcoreguidelines-const.WarnPointersAsValues", value: 1}]}' \
+// 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
+
+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 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 p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int *const p0_p_local0 = _local0;
+  int *const p1_p_local0 = _local0;
+
+  int np_local0 = 42;
+  function_inout_pointer(_local0);
+
+  // Prevents const.
+  int np_local1 = 42;
+  out = _local1; // 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 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int' can be declared const
+  function_in_pointer(_local2);
+}
+
+void function_inout_ref(int );
+void function_in_ref(const int );
+
+void some_reference_taking() {
+  // FIXME False positive
+  int np_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'np_local0' of type 'int' can be declared const
+  const int _np_local0 = np_local0;
+  int _np_local0 = np_local0;
+  r1_np_local0 = 43;
+  const int _np_local0 = r1_np_local0;
+
+  int np_local1 = 42;
+  function_inout_ref(np_local1);
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int _p_local0 = p_local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_ref(p_local1);
+}
+
+double *non_const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double np_local0 = 24.4;
+
+  return _local0;
+}
+
+const double *const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double p_local1 = 24.4;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' 

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

@shuaiwang I implemented the check on top of you utility function. It does fail 
right now (concentrating on values only for now). I added a `FIXME` for all 
false positives/negatives for the values test cases.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- [Feature] refactor check to use a utility function to determine constness


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/ConstCheck.cpp
  clang-tidy/cppcoreguidelines/ConstCheck.h
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-const.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-const-handles.cpp
  test/clang-tidy/cppcoreguidelines-const-values.cpp

Index: test/clang-tidy/cppcoreguidelines-const-values.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-const-values.cpp
@@ -0,0 +1,418 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: "cppcoreguidelines-const.AnalyzeValues", value: 1},\
+// RUN:   {key: "cppcoreguidelines-const.AnalyzeHandles", value: 0},\
+// RUN:   {key: "cppcoreguidelines-const.WarnPointersAsValues", value: 1}]}' \
+// 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
+
+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 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 p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int *const p0_p_local0 = _local0;
+  int *const p1_p_local0 = _local0;
+
+  int np_local0 = 42;
+  function_inout_pointer(_local0);
+
+  // Prevents const.
+  int np_local1 = 42;
+  out = _local1; // 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 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local2' of type 'int' can be declared const
+  function_in_pointer(_local2);
+}
+
+void function_inout_ref(int );
+void function_in_ref(const int );
+
+void some_reference_taking() {
+  int np_local0 = 42;
+  const int _np_local0 = np_local0;
+  int _np_local0 = np_local0;
+  r1_np_local0 = 43;
+  const int _np_local0 = r1_np_local0;
+
+  int np_local1 = 42;
+  function_inout_ref(np_local1);
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int _p_local0 = p_local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_ref(p_local1);
+}
+
+double *non_const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double np_local0 = 24.4;
+
+  return _local0;
+}
+
+const double *const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double p_local1 = 24.4;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' can be declared const
+  return _local1;
+}
+
+double _const_ref_return() {
+  double 

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In https://reviews.llvm.org/D45444#1069488, @shuaiwang wrote:

> I've updated https://reviews.llvm.org/D45679 and I think the `isModified()` 
> function there is now sufficiently covering the cases you've covered here and 
> can be used as a good starting version if you plan to use it here.
>  I copied your const-values test cases and it now passes with the following 
> differences:
>
> - All unused local variables removed, my check will issue a warning for them 
> because technically they're not modified, but I understand why you don't want 
> to cover them. I don't feel strongly about it and I removed it from my 
> copy-pasted test cases because I just to demonstrate `isModified()`


Nice!

> - Better recall in `some_reference_taking`, both `np_local0` and 
> `r1_np_local0` can be caught, similar for `np_local1` and `non_const_ref` in 
> `handle_from_array`.

Yes. But i feel we need to gather more information. For potential checks, it 
might be nice to control to which scope modification is defined and/or emit 
`note`s for interesting events like taking a non-const handle which isnt 
modified, too.

It kinda feels like we are converging to a dependency graph for every variable 
that contains the information for dependencies and dependents + modification 
history. I dont think it is bad, but maybe complicated :)
What do you think about returning a `ModificationRecord` that contains the 
number of direct modification, the number of local non-const handles and 
escaping non-const handles. 
Including const handles would be nice in the future for more and different 
analysis.

> - `direct_class_access` `np_local5` triggers with my check (shouldn't it?)

Yes can be const: Godbolt 


> - In `range_for` non-const loop variables triggers with my check (shouldn't 
> they?)



- If the loop copies every value no modification occurs (its not a copy, its 
initialization. If the constructor would modify its value, the source cant be 
const either. Is it allowed?)
- if a handle is taken the usual rules apply.

> - In `range_for` local arrays doesn't trigger with my check, because I'm 
> treating ArrayToPointerDecay the same as taking address of a variable, and 
> looping over an array would involve ArrayToPointerDecay when the implicit 
> `__begin`/`__end` are initialized. I added a note inside `isModified` for 
> future improvements.

Given this is implemented here, can you use it?

> - My check over triggers for template (with my failed attempt to fix), but I 
> think it's more of some mistake in the check itself rather than in 
> `isModified`

I removed templates while matching for potential candidates. That can be moved 
out of the `isModified()`. `isModified()` can only reason about instantiated 
functions. For the actual template we need concepts.

> Sorry about the confusion.
>  Basically consider this example:
> 
>   class Foo {
>   public:
> void a() { x = 10; }
> void b() { // nothing }
> void c() { a(); }
> void d() { b(); }
> 
>   private:
> int x;
>   };
> 
> 
> If we check whether `isModified(dereference(cxxThisExpr()))` for each 
> `CompondStmt(hasParent(functionDecl()))`, we would conclude that:
> 
> - `a()` should stay non-const, because there's `this->x = 10` modifying 
> `*this`
> - `b()` should be changed to const, because nothing modifies `*this`
> - `c()` should stay non-const, because we're invoking non-const member 
> function on `this`
> - `d()` should also stay 

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang added a comment.

I've updated https://reviews.llvm.org/D45679 and I think the `isModified()` 
function there is now sufficiently covering the cases you've covered here and 
can be used as a good starting version if you plan to use it here.
I copied your const-values test cases and it now passes with the following 
differences:

- All unused local variables removed, my check will issue a warning for them 
because technically they're not modified, but I understand why you don't want 
to cover them. I don't feel strongly about it and I removed it from my 
copy-pasted test cases because I just to demonstrate `isModified()`
- Better recall in `some_reference_taking`, both `np_local0` and `r1_np_local0` 
can be caught, similar for `np_local1` and `non_const_ref` in 
`handle_from_array`.
- `direct_class_access` `np_local5` triggers with my check (shouldn't it?)
- In `range_for` non-const loop variables triggers with my check (shouldn't 
they?)
- In `range_for` local arrays doesn't trigger with my check, because I'm 
treating ArrayToPointerDecay the same as taking address of a variable, and 
looping over an array would involve ArrayToPointerDecay when the implicit 
`__begin`/`__end` are initialized. I added a note inside `isModified` for 
future improvements.
- My check over triggers for template (with my failed attempt to fix), but I 
think it's more of some mistake in the check itself rather than in `isModified`

>> Also when marking decls as "can't be const" we'll have to do record 
>> separately for modifying behaviors coming from different functions.
>>  Which of course are all possible but code will get too complex than 
>> necessary IMO.
> 
> I cant follow you on that one. I consider every path that allows future 
> modification (like taking a non-const reference/pointer) as `cant be const`. 
> That would be enough, wouldnt it?
>  A separate function can only modify a variable if it has some form of 
> non-const handle to it, which must have been created at some point.

Sorry about the confusion.
Basically consider this example:

  class Foo {
  public:
void a() { x = 10; }
void b() { // nothing }
void c() { a(); }
void d() { b(); }

  private:
int x;
  };

If we check whether `isModified(dereference(cxxThisExpr()))` for each 
`CompondStmt(hasParent(functionDecl()))`, we would conclude that:

- `a()` should stay non-const, because there's `this->x = 10` modifying `*this`
- `b()` should be changed to const, because nothing modifies `*this`
- `c()` should stay non-const, because we're invoking non-const member function 
on `this`
- `d()` should also stay non-const, with the same reason for c(). (We can in 
theory be smarter about this one because the definition of b() happens to be 
inside the same TU but that's out of scope right now)

However if we use a per-TU map to record whether `x` can be const, we'll 
conclude that `x` is modified thus can't be const, missing the one that `b()` 
is not modifying `x`. To fix that we'll need two-layered map recording that `x` 
is only modified in `a()` and potentially modified indirectly from `c()` and 
`d()`, so that in the end we can figure out that `b()` is safe to be marked 
const.

Anyway, all I was trying to say is: let's use the `isModified()` approach as 
it's simpler & cleaner for future use cases like adding const to member 
functions. And it feels to me that we've already reached agreement there :)

> @shuaiwang Are you in IRC from time to time? I think it will be better to 
> discuss changes in chat, when i start to introduce your approach here.

Not really, but I can get myself familiar with it.

> 
> 
>> isModified(Expr, Stmt), that checks whether Expr is (potentially) modified 
>> within Stmt, is something I believe to be quite useful:
> 
> What i understand from your comments and code:
> 
> - implement a utility taking `DeclRefExpr` and check if there is modification 
> in some scope (`Stmt`, e.g. `CompoundStmt`) -> `isModified` or 
> `cantBeModifiedWithin`

It doesn't have to be `DeclRefExpr`, any `Expr` should work, and it's useful to 
make it accept any `Expr`:

- It can be used to check whether `dereference(cxxThisExpr())` is modified or 
not.
- For pointer types, it can be used to check both 
`declRefExpr(isPointerType())` and `dereference(declRefExpr(isPointerType()))`, 
and suggest adding const at different level

> - match all relevant non-const variable declarations as potential candidates
> - track all uses of the candidates and check for modification in their scope
> 
>   One Note: We could include variables in unnamed namespaces and `static` 
> declared variables. They have TU scope.

Great catch!

> My deviations:
> 
> - a variable should be considered modified if a non-const handle is create 
> from it, even if that handle does not modify its referenced value. As first 
> step, those handles should be marked const, then the original value can be 
> marked as const. That is required to produce compiling 

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> You'll also need to check:
> 
> - a member function calling another non-const member function -> *this can't 
> be const
> - *this is passed as non-const reference param to a function -> *this can't 
> be const

Yes. That is similar behaviour to checking if a function can be `noexcept`. But 
this is all future stuff :)

> Also when marking decls as "can't be const" we'll have to do record 
> separately for modifying behaviors coming from different functions.
>  Which of course are all possible but code will get too complex than 
> necessary IMO.

I cant follow you on that one. I consider every path that allows future 
modification (like taking a non-const reference/pointer) as `cant be const`. 
That would be enough, wouldnt it?
A separate function can only modify a variable if it has some form of non-const 
handle to it, which must have been created at some point.

> I think member variables are a separate topic: I think we should just treat 
> them as globals and not check on them, the same argument that they can be 
> accessed from multiple translation unit applies to global/namespace scoped 
> variables and class scoped variables. We can only reliably check 
> function/block scope variables.
>  (reliable meaning being able to achieve zero false positives with useful 
> level of recall, false negatives are inevitable because whenever a modifiable 
> reference/handle escape the current block/translation unit we'll have to 
> assume it's modified.)

You are probably right. The only zero-false positive case is: "only const 
methods called". One could split implementation of a class into several 
translation units and render the analysis approach useless.

> Yes my approach is doing multi-pass matching by calling isModified() 
> recursively. I consider the multi-pass matching "necessary evil" because 
> otherwise we'll have too many false negatives.
>  I thought about hasDescendent (hasAncestor actually) but I don't think that 
> makes things easier: varDeclRef(hasAncestor(modifying)) would match both 
> "v.a.b.c. = 10" and "map[v] = 10" and we'll still need to double check the 
> modifying behavior does link back to the particular varDeclRef.

Example as reference for later: https://godbolt.org/g/cvhoUN
I will add such cases to my tests.

@shuaiwang Are you in IRC from time to time? I think it will be better to 
discuss changes in chat, when i start to introduce your approach here.

> isModified(Expr, Stmt), that checks whether Expr is (potentially) modified 
> within Stmt, is something I believe to be quite useful:

What i understand from your comments and code:

- implement a utility taking `DeclRefExpr` and check if there is modification 
in some scope (`Stmt`, e.g. `CompoundStmt`) -> `isModified` or 
`cantBeModifiedWithin`
- match all relevant non-const variable declarations as potential candidates
- track all uses of the candidates and check for modification in their scope

One Note: We could include variables in unnamed namespaces and `static` 
declared variables. They have TU scope.

My deviations:

- a variable should be considered modified if a non-const handle is create from 
it, even if that handle does not modify its referenced value. As first step, 
those handles should be marked const, then the original value can be marked as 
const. That is required to produce compiling code after potential 
code-transformation.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang added a comment.

In https://reviews.llvm.org/D45444#1068967, @Eugene.Zelenko wrote:

> In https://reviews.llvm.org/D45444#1068496, @shuaiwang wrote:
>
> > - I would imagine things could get messier if this check expands to also 
> > check for turning member functions const: it's basically checking 
> > CxxThisExpr, being a handle, is not modified within a member function, but 
> > note there's no VarDecl for "this".
>
>
> Probably this should be separate check. See also PR21981.


Sure, a separate check sounds better.
Which makes even strong argument of having a reusable utility checking whether 
something is modified that can be shared between different checks :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang added a comment.

>>   I would imagine things could get messier if this check expands to also 
>> check for turning member functions const: it's basically checking 
>> CxxThisExpr, being a handle, is not modified within a member function, but 
>> note there's no VarDecl for "this".
> 
> Using my approach, you can check if any member variable is used as non-const. 
> Then mark this method as const, if it is not virtual.
>  Similar for member variables: private non-consts can be converted into 
> consts.

You'll also need to check:

- a member function calling another non-const member function -> *this can't be 
const
- *this is passed as non-const reference param to a function -> *this can't be 
const

Also when marking decls as "can't be const" we'll have to do record separately 
for modifying behaviors coming from different functions.
Which of course are all possible but code will get too complex than necessary 
IMO.

I think member variables are a separate topic: I think we should just treat 
them as globals and not check on them, the same argument that they can be 
accessed from multiple translation unit applies to global/namespace scoped 
variables and class scoped variables. We can only reliably check function/block 
scope variables.
(reliable meaning being able to achieve zero false positives with useful level 
of recall, false negatives are inevitable because whenever a modifiable 
reference/handle escape the current block/translation unit we'll have to assume 
it's modified.)

>>   It's also cleaner to follow member access chains, e.g. "v.a.b.c = 10" 
>> modifies "v". This one is particularly tricky because the modifying behavior 
>> happens at "c = 10" while the variable we'd like to mark as "can't be const" 
>> is arbitrary layers away, and MemberExpr alone doesn't warrant marking 
>> "can't be const" because that'll introduce way too many false negatives, ... 
>> and I haven't figure out a way to write a matcher to match 
>> memberExpr(hasObjectExpression(memberExpr(hasObjectExpression(... > layers deep> ... (VarDeclRef) ...), not to mention any layer could either 
>> member access or array subscript access.
> 
> Does your approach work with the nesting? Maybe something recursive or 
> `hasDescendent(modifying)` could do it?

Yes my approach is doing multi-pass matching by calling isModified() 
recursively. I consider the multi-pass matching "necessary evil" because 
otherwise we'll have too many false negatives.
I thought about hasDescendent (hasAncestor actually) but I don't think that 
makes things easier: varDeclRef(hasAncestor(modifying)) would match both 
"v.a.b.c. = 10" and "map[v] = 10" and we'll still need to double check the 
modifying behavior does link back to the particular varDeclRef.

Reviewers, what are your thoughts about the approaches?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D45444#1068496, @shuaiwang wrote:

> - I would imagine things could get messier if this check expands to also 
> check for turning member functions const: it's basically checking 
> CxxThisExpr, being a handle, is not modified within a member function, but 
> note there's no VarDecl for "this".


Probably this should be separate check. See also PR21981.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> Coming from https://reviews.llvm.org/D45679, which I should have sent out 
> much earlier to get in front of you :p
> 
> Kidding aside this check is more mature than mine and I'm happy to help 
> incorporate mine into this one.

I would love to, yours is more elegant :)

> I do have a strong opinion that requires non-trivial refactoring of your diff 
> though: having a standalone reusable helper function like isModified(Expr, 
> Stmt), that checks whether Expr is (potentially) modified within Stmt, is 
> something I believe to be quite useful:

Yes please!

>   performance/{ForRangeCopyCheck, UnnecessaryCopyInitialization} are using a 
> much less sophisticated isOnlyUsedAsConst(), and can benefit from a more 
> powerful one.

I did not think about such possibilities, but maybe other analysis task could 
benefit too? Warnings?

>   I would imagine things could get messier if this check expands to also 
> check for turning member functions const: it's basically checking 
> CxxThisExpr, being a handle, is not modified within a member function, but 
> note there's no VarDecl for "this".

Using my approach, you can check if any member variable is used as non-const. 
Then mark this method as const, if it is not virtual.
Similar for member variables: private non-consts can be converted into consts.

>   It's cleaner to follow (non-const) reference chains and only mark a decl as 
> "can't be const" if everything on the chain is not modified, avoiding false 
> negatives when a variable is bound to a never-modified 
>   non-const reference.

For warning only yes. But i want automatic code transformation which will break 
such code if there are non-const references taken. Having it in one pass is 
certainly nice. With my approach multiple runs are required if the handles are 
marked as const.

>   It's also cleaner to follow member access chains, e.g. "v.a.b.c = 10" 
> modifies "v". This one is particularly tricky because the modifying behavior 
> happens at "c = 10" while the variable we'd like to mark as "can't be const" 
> is arbitrary layers away, and MemberExpr alone doesn't warrant marking "can't 
> be const" because that'll introduce way too many false negatives, ... and I 
> haven't figure out a way to write a matcher to match 
> memberExpr(hasObjectExpression(memberExpr(hasObjectExpression(...  layers deep> ... (VarDeclRef) ...), not to mention any layer could either 
> member access or array subscript access.

Does your approach work with the nesting? Maybe something recursive or 
`hasDescendent(modifying)` could do it?

Is read your comment on this check later, i first saw your new check. Maybe we 
should discuss only under one of both :) (which one is not important for me :))


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Shuai Wang via Phabricator via cfe-commits]

shuaiwang added a comment.

Coming from https://reviews.llvm.org/D45679, which I should have sent out much 
earlier to get in front of you :p

Kidding aside this check is more mature than mine and I'm happy to help 
incorporate mine into this one.

I do have a strong opinion that requires non-trivial refactoring of your diff 
though: having a standalone reusable helper function like isModified(Expr, 
Stmt), that checks whether Expr is (potentially) modified within Stmt, is 
something I believe to be quite useful:

- performance/{ForRangeCopyCheck, UnnecessaryCopyInitialization} are using a 
much less sophisticated isOnlyUsedAsConst(), and can benefit from a more 
powerful one.
- I would imagine things could get messier if this check expands to also check 
for turning member functions const: it's basically checking CxxThisExpr, being 
a handle, is not modified within a member function, but note there's no VarDecl 
for "this".
- It's cleaner to follow (non-const) reference chains and only mark a decl as 
"can't be const" if everything on the chain is not modified, avoiding false 
negatives when a variable is bound to a never-modified non-const reference.
- It's also cleaner to follow member access chains, e.g. "v.a.b.c = 10" 
modifies "v". This one is particularly tricky because the modifying behavior 
happens at "c = 10" while the variable we'd like to mark as "can't be const" is 
arbitrary layers away, and MemberExpr alone doesn't warrant marking "can't be 
const" because that'll introduce way too many false negatives, ... and I 
haven't figure out a way to write a matcher to match 
memberExpr(hasObjectExpression(memberExpr(hasObjectExpression(...  ... (VarDeclRef) ...), not to mention any layer could either 
member access or array subscript access.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

@aaron.ballman @lebedev.ri The check is getting really complex. I run it over 
LLVM and detected some warnings, where iam not sure if they are valid or not. 
Its already a somewhat usable state, but its hard to determine false positives 
and false negatives.

For me, that false positives are worse for now, because they annoy. Its easier 
to add false negatives especially with user feedback. My idea is now: Take a 
look at refactoring the code to introduce const as fixit and iterate the check 
until LLVM compiles after the check fixed all missing consts. What do you think 
about that strategy? And could you take a look at the todo list at the top of 
`ConstCheck.cpp`. Did I miss something?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- [Misc] unify handle and value modification detection
- [Misc] found new caveats, maybe take a look at refactoring first an require 
this check to produce clean compiles on llvm


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/ConstCheck.cpp
  clang-tidy/cppcoreguidelines/ConstCheck.h
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-const.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-const-handles.cpp
  test/clang-tidy/cppcoreguidelines-const-values.cpp

Index: test/clang-tidy/cppcoreguidelines-const-values.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-const-values.cpp
@@ -0,0 +1,416 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: "cppcoreguidelines-const.AnalyzeValues", value: 1},\
+// RUN:   {key: "cppcoreguidelines-const.AnalyzeHandles", value: 0},\
+// RUN:   {key: "cppcoreguidelines-const.WarnPointersAsValues", value: 1}]}' \
+// 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
+
+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 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;
+  function_inout_pointer(_local1);
+
+  // Prevents const.
+  int np_local2 = 42;
+  out = _local2; // This returns and invalid address, its just about the AST
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int *const p0_p_local0 = _local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_pointer(_local1);
+}
+
+void function_inout_ref(int );
+void function_in_ref(const int );
+
+void some_reference_taking() {
+  int np_local0 = 42;
+  const int _np_local0 = np_local0;
+  int _np_local0 = np_local0;
+  const int _np_local0 = r1_np_local0;
+
+  int np_local1 = 42;
+  function_inout_ref(np_local1);
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int _p_local0 = p_local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_ref(p_local1);
+}
+
+double *non_const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double np_local0 = 24.4;
+
+  return _local0;
+}
+
+const double *const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double p_local1 = 24.4;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' can be declared const
+  return _local1;
+}
+
+double _const_ref_return() {
+  double p_local0 = 0.0;
+  

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- [Feature] start working on handle semantic


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/ConstCheck.cpp
  clang-tidy/cppcoreguidelines/ConstCheck.h
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-const.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-const-handles.cpp
  test/clang-tidy/cppcoreguidelines-const-values.cpp

Index: test/clang-tidy/cppcoreguidelines-const-values.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-const-values.cpp
@@ -0,0 +1,392 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: "cppcoreguidelines-const.AnalyzeValues", value: 1},\
+// RUN:   {key: "cppcoreguidelines-const.AnalyzeHandles", value: 0},\
+// RUN:   {key: "cppcoreguidelines-const.WarnPointersAsValues", value: 1}]}' \
+// 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
+
+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 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;
+  function_inout_pointer(_local1);
+
+  // Prevents const.
+  int np_local2 = 42;
+  out = _local2; // This returns and invalid address, its just about the AST
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int *const p0_p_local0 = _local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_pointer(_local1);
+}
+
+void function_inout_ref(int );
+void function_in_ref(const int );
+
+void some_reference_taking() {
+  int np_local0 = 42;
+  const int _np_local0 = np_local0;
+  int _np_local0 = np_local0;
+  const int _np_local0 = r1_np_local0;
+
+  int np_local1 = 42;
+  function_inout_ref(np_local1);
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int _p_local0 = p_local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_ref(p_local1);
+}
+
+double *non_const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double np_local0 = 24.4;
+
+  return _local0;
+}
+
+const double *const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double p_local1 = 24.4;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' can be declared const
+  return _local1;
+}
+
+double _const_ref_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double np_local0 = 42.42;
+  

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

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

- [Feature] implement array and method access
- [Feature] finalize value semantic


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/ConstCheck.cpp
  clang-tidy/cppcoreguidelines/ConstCheck.h
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-const.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-const-primtive-builtin.cpp

Index: test/clang-tidy/cppcoreguidelines-const-primtive-builtin.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-const-primtive-builtin.cpp
@@ -0,0 +1,387 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const %t
+
+// --- 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
+
+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 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;
+  function_inout_pointer(_local1);
+
+  // Prevents const.
+  int np_local2 = 42;
+  out = _local2; // This returns and invalid address, its just about the AST
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int *const p0_p_local0 = _local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_pointer(_local1);
+}
+
+void function_inout_ref(int );
+void function_in_ref(const int );
+
+void some_reference_taking() {
+  int np_local0 = 42;
+  const int _np_local0 = np_local0;
+  int _np_local0 = np_local0;
+  const int _np_local0 = r1_np_local0;
+
+  int np_local1 = 42;
+  function_inout_ref(np_local1);
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int _p_local0 = p_local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_ref(p_local1);
+}
+
+double *non_const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double np_local0 = 24.4;
+
+  return _local0;
+}
+
+const double *const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double p_local1 = 24.4;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'double' can be declared const
+  return _local1;
+}
+
+double _const_ref_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double np_local0 = 42.42;
+  return np_local0;
+}
+
+const double _ref_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double p_local1 = 24.4;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: 

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Thank you for working on this check!




Comment at: clang-tidy/cppcoreguidelines/ConstCheck.h:60
+
+  std::unordered_map ValueCanBeConst;
+  std::unordered_map HandleCanBeConst;

I'm guessing these should be `llvm::DenseMap`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

In https://reviews.llvm.org/D45444#1063291, @JonasToth wrote:

> > It'll be good idea to have option to apply this check for 
> > pointer/references only, or include built-in types/enums.
>
> Agreed. I aim at a `mark handles const`(default on), `mark values 
> const`(default on for objects), `mark pointer (const int * >> const <<) 
> const` (default off)
>
> What do you mean by built-in types/enums? That they are configurable separate 
> to objects?


I meant integers/floating point numbers/Boolean/enums. It may be overkill to 
apply this check for legacy code base in single pass.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> It'll be good idea to have option to apply this check for pointer/references 
> only, or include built-in types/enums.

Agreed. I aim at a `mark handles const`(default on), `mark values 
const`(default on for objects), `mark pointer (const int * >> const <<) const` 
(default off)

What do you mean by built-in types/enums? That they are configurable separate 
to objects?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

Will be good idea to add HICPP alias.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added a comment.

It'll be good idea to have option to apply this check for pointer/references 
only, or include built-in types/enums.




Comment at: clang-tidy/cppcoreguidelines/ConstCheck.cpp:13
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+#include 

Please remove empty line.



Comment at: docs/ReleaseNotes.rst:60
 
+- New :doc:`cppcoreguidelines-const
+  ` check

Please move into new checks list in alphabetical order.



Comment at: docs/ReleaseNotes.rst:63
+
+  FIXME: add release notes.
+

Please add short description here.



Comment at: docs/clang-tidy/checks/cppcoreguidelines-const.rst:6
+
+FIXME: Describe what patterns does the check detect and why. Give examples.

Preliminary documentation will be helpful.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

@aaron.ballman, @lebedev.ri, @alexfh and everyone else, i would love to hear 
your opinion on my approach. i never implemented a similar check and i might 
miss some cases, that need to be considered.

The current state is just a proof-of-concept to see how such a check could be 
implemented.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444



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

[PATCH] D45444: [clang-tidy] WIP: implement new check for const-correctness

[Jonas Toth via Phabricator via cfe-commits]

JonasToth created this revision.
JonasToth added reviewers: alexfh, aaron.ballman, lebedev.ri, hokein.
Herald added subscribers: cfe-commits, kbarton, xazax.hun, mgorny, nemanjai, 
klimek.

This check aims to determine values and references that could be declared
as const, but are not.

The first version I am aiming at only analyzes local variables.
No fixits are emitted.

This check aims to implement `CPPCG-ES. 25: Declare an object const or 
constexpr unless you want to modify its value later on 
`
and `HICPP-7.1.2 Use const whenever possible 
`.

Because const-correctness includes many issues this check will be implemented
in multiple stages.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D45444

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/ConstCheck.cpp
  clang-tidy/cppcoreguidelines/ConstCheck.h
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-const.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-const-primtive-builtin.cpp

Index: test/clang-tidy/cppcoreguidelines-const-primtive-builtin.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-const-primtive-builtin.cpp
@@ -0,0 +1,187 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-const %t
+
+// --- 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
+
+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 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;
+  function_inout_pointer(_local1);
+
+  // Prevents const.
+  int np_local2 = 42;
+  out = _local2; // This returns and invalid address, its just about the AST
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int *const p0_p_local0 = _local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_pointer(_local1);
+}
+
+void function_inout_ref(int );
+void function_in_ref(const int );
+
+void some_reference_taking() {
+  int np_local0 = 42;
+  const int _np_local0 = np_local0;
+  int _np_local0 = np_local0;
+  const int _np_local0 = r1_np_local0;
+
+  int np_local1 = 42;
+  function_inout_ref(np_local1);
+
+  int p_local0 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'int' can be declared const
+  const int _p_local0 = p_local0;
+
+  int p_local1 = 42;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local1' of type 'int' can be declared const
+  function_in_ref(p_local1);
+}
+
+double *non_const_pointer_return() {
+  double p_local0 = 0.0;
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: variable 'p_local0' of type 'double' can be declared const
+  double np_local0 = 24.4;
+
+  return _local0;
+}
+
+const double *const_pointer_return() {
+