[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D54395#1803552 , @thakis wrote:

> This breaks tests on Windows: http://45.33.8.238/win/5061/step_8.txt
>
> Please take a look, and if takes a while to investigate, please revert in the 
> meantime. (Probably needs the fno-delayed-template-instantiation kludge 
> that's in many other tests.)


you are right. I belive it is "-fno-delayed-template-parsing" though. I added 
it to the default arguments `runCheckOnCode` for quickfix.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

This breaks tests on Windows: http://45.33.8.238/win/5061/step_8.txt

Please take a look, and if takes a while to investigate, please revert in the 
meantime. (Probably needs the fno-delayed-template-instantiation kludge that's 
in many other tests.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGcf48101200ee: [clang-tidy] implement utility-function to add 
const to variables (authored by JonasToth).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395

Files:
  clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.h
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.h
  clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt

Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -7,6 +7,7 @@
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
+  AddConstTest.cpp
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
@@ -0,0 +1,1081 @@
+#include "../clang-tidy/utils/FixItHintUtils.h"
+#include "ClangTidyDiagnosticConsumer.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+using namespace clang::ast_matchers;
+using namespace utils::fixit;
+
+template 
+class ConstTransform : public ClangTidyCheck {
+public:
+  ConstTransform(StringRef CheckName, ClangTidyContext *Context)
+  : ClangTidyCheck(CheckName, Context) {}
+
+  void registerMatchers(MatchFinder *Finder) override {
+Finder->addMatcher(varDecl(hasName("target")).bind("var"), this);
+  }
+
+  void check(const MatchFinder::MatchResult ) override {
+const auto *D = Result.Nodes.getNodeAs("var");
+using utils::fixit::addQualifierToVarDecl;
+Optional Fix = addQualifierToVarDecl(
+*D, *Result.Context, DeclSpec::TQ::TQ_const, CT, CP);
+auto Diag = diag(D->getBeginLoc(), "doing const transformation");
+if (Fix)
+  Diag << *Fix;
+  }
+};
+} // namespace
+
+namespace test {
+using PointeeLTransform =
+ConstTransform;
+using PointeeRTransform =
+ConstTransform;
+
+using ValueLTransform =
+ConstTransform;
+using ValueRTransform =
+ConstTransform;
+
+// 
+// Test Value-like types. Everything with indirection is done later.
+// 
+
+TEST(Values, Builtin) {
+  StringRef Snippet = "int target = 0;";
+
+  EXPECT_EQ("const int target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("const int target = 0;",
+runCheckOnCode(Snippet));
+
+  EXPECT_EQ("int const target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("int const target = 0;",
+runCheckOnCode(Snippet));
+}
+TEST(Values, TypedefBuiltin) {
+  StringRef T = "typedef int MyInt;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, TypedefBuiltinPointer) {
+  StringRef T = "typedef int* MyInt;";
+  StringRef S = "MyInt target = nullptr;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltin) {
+  StringRef T = "using MyInt = int;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+ 

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

Thank you for all the review over the time this took to land! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:1055
+  EXPECT_NE(runCheckOnCode(Cat(S), nullptr, "input.m"),
+Cat("Object  const*target;"));
+  EXPECT_NE(runCheckOnCode(Cat(S), nullptr, "input.m"),

JonasToth wrote:
> aaron.ballman wrote:
> > The whitespace looks incorrect here.
> It does, but it is actually how it is expected to look.
> All pointer transformations create a double-blank if there was a blank before 
> the star. My thought was, that clang-format should be used anyway, so it 
> should do this cleanup. (especially with the -format option for clang-tidy).
> The additional whitespace needs to be inserted to avoid `intconst* target;` 
> for `int* target;` variables. Thats why it is always added, if necessary or 
> not.
> It does, but it is actually how it is expected to look.

Okay, that's fair -- we usually let clang-format handle that sort of thing 
anyway and I cannot think of a case where the transformation would be incorrect.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:1055
+  EXPECT_NE(runCheckOnCode(Cat(S), nullptr, "input.m"),
+Cat("Object  const*target;"));
+  EXPECT_NE(runCheckOnCode(Cat(S), nullptr, "input.m"),

aaron.ballman wrote:
> The whitespace looks incorrect here.
It does, but it is actually how it is expected to look.
All pointer transformations create a double-blank if there was a blank before 
the star. My thought was, that clang-format should be used anyway, so it should 
do this cleanup. (especially with the -format option for clang-tidy).
The additional whitespace needs to be inserted to avoid `intconst* target;` for 
`int* target;` variables. Thats why it is always added, if necessary or not.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Still LG with a small nit in one of the FIXME tests.




Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:1006
+}
+
+} // namespace test

JonasToth wrote:
> aaron.ballman wrote:
> > JonasToth wrote:
> > > aaron.ballman wrote:
> > > > Can you also add some ObjC pointer tests?
> > > i added the most basic test, do you think more is necessary? I don't know 
> > > a lot about the objc languages.
> > Ah, those are regular C pointers, not ObjC pointers. I was thinking more 
> > along the lines of:
> > ```
> > @class Object;
> > Object *g; // Try adding const to this
> > 
> > @interface Inter
> > - (void) foo1: (int *)arg1; // Try adding const to this parameter
> > @end
> > ```
> Well, those don't work well.
> Can i still commit? :) FIXMEs are there.
> Can i still commit? :) FIXMEs are there.

Yes, I think you've still made good progress with the patch as-is. We can 
correct the ObjC stuff in a follow-up.



Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:1055
+  EXPECT_NE(runCheckOnCode(Cat(S), nullptr, "input.m"),
+Cat("Object  const*target;"));
+  EXPECT_NE(runCheckOnCode(Cat(S), nullptr, "input.m"),

The whitespace looks incorrect here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:1006
+}
+
+} // namespace test

aaron.ballman wrote:
> JonasToth wrote:
> > aaron.ballman wrote:
> > > Can you also add some ObjC pointer tests?
> > i added the most basic test, do you think more is necessary? I don't know a 
> > lot about the objc languages.
> Ah, those are regular C pointers, not ObjC pointers. I was thinking more 
> along the lines of:
> ```
> @class Object;
> Object *g; // Try adding const to this
> 
> @interface Inter
> - (void) foo1: (int *)arg1; // Try adding const to this parameter
> @end
> ```
Well, those don't work well.
Can i still commit? :) FIXMEs are there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

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

- add proper objc test, that fail mostly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395

Files:
  clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.h
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.h
  clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt

Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -7,6 +7,7 @@
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
+  AddConstTest.cpp
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
@@ -0,0 +1,1081 @@
+#include "../clang-tidy/utils/FixItHintUtils.h"
+#include "ClangTidyDiagnosticConsumer.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+using namespace clang::ast_matchers;
+using namespace utils::fixit;
+
+template 
+class ConstTransform : public ClangTidyCheck {
+public:
+  ConstTransform(StringRef CheckName, ClangTidyContext *Context)
+  : ClangTidyCheck(CheckName, Context) {}
+
+  void registerMatchers(MatchFinder *Finder) override {
+Finder->addMatcher(varDecl(hasName("target")).bind("var"), this);
+  }
+
+  void check(const MatchFinder::MatchResult ) override {
+const auto *D = Result.Nodes.getNodeAs("var");
+using utils::fixit::addQualifierToVarDecl;
+Optional Fix = addQualifierToVarDecl(
+*D, *Result.Context, DeclSpec::TQ::TQ_const, CT, CP);
+auto Diag = diag(D->getBeginLoc(), "doing const transformation");
+if (Fix)
+  Diag << *Fix;
+  }
+};
+} // namespace
+
+namespace test {
+using PointeeLTransform =
+ConstTransform;
+using PointeeRTransform =
+ConstTransform;
+
+using ValueLTransform =
+ConstTransform;
+using ValueRTransform =
+ConstTransform;
+
+// 
+// Test Value-like types. Everything with indirection is done later.
+// 
+
+TEST(Values, Builtin) {
+  StringRef Snippet = "int target = 0;";
+
+  EXPECT_EQ("const int target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("const int target = 0;",
+runCheckOnCode(Snippet));
+
+  EXPECT_EQ("int const target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("int const target = 0;",
+runCheckOnCode(Snippet));
+}
+TEST(Values, TypedefBuiltin) {
+  StringRef T = "typedef int MyInt;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, TypedefBuiltinPointer) {
+  StringRef T = "typedef int* MyInt;";
+  StringRef S = "MyInt target = nullptr;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltin) {
+  StringRef T = "using MyInt = int;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltinPointer) {
+  

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:66
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+

JonasToth wrote:
> aaron.ballman wrote:
> > I don't think this is needed -- just add a newline between the literals and 
> > let string concat work its magic? (Same elsewhere)
> > ```
> > StringRef S = "typedef int MyInt;"
> >   "MyInt target = 0;";
> > ```
> My Idea was to highlight the expected code-change and create both the before 
> and after-version from the same snippets.
> With `S` being all the code, i would need to test for the full code.
> 
> I think with your proposal the test looks like this:
> 
> ```
> StringRef S = "typedef int MyInt;"
>   "MyInt target = 0;";
> EXPECT_EQ("typedef int MyInt;"
>   "const MyInt target = 0;",
>   runCheckOnCode(S));
> ```
> I prefer the current version, especially with the bigger test later this file.
Okay, that's a good point. Also, this is testing code, so if it's a bit... 
odd... it's not an issue.



Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:83-84
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),

JonasToth wrote:
> aaron.ballman wrote:
> > This does what I would expect, but boy does it make me sad to see (because 
> > `target` is not actually a `const int *` but is instead an `int * const`).
> You are right, but I am not sure what the proper policy should be. I think 
> the smarts to detect such potential improper const should be in the library 
> code calling the transformation.
> The utility should allow both transformations.
I think the API behaves how I would expect it to and agree that the caller 
should be the one deciding what to add the qualifier to. It just hurts me to 
see the test code verifying it (but the test code is good). ;-)



Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:1006
+}
+
+} // namespace test

JonasToth wrote:
> aaron.ballman wrote:
> > Can you also add some ObjC pointer tests?
> i added the most basic test, do you think more is necessary? I don't know a 
> lot about the objc languages.
Ah, those are regular C pointers, not ObjC pointers. I was thinking more along 
the lines of:
```
@class Object;
Object *g; // Try adding const to this

@interface Inter
- (void) foo1: (int *)arg1; // Try adding const to this parameter
@end
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

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



Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:66
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+

aaron.ballman wrote:
> I don't think this is needed -- just add a newline between the literals and 
> let string concat work its magic? (Same elsewhere)
> ```
> StringRef S = "typedef int MyInt;"
>   "MyInt target = 0;";
> ```
My Idea was to highlight the expected code-change and create both the before 
and after-version from the same snippets.
With `S` being all the code, i would need to test for the full code.

I think with your proposal the test looks like this:

```
StringRef S = "typedef int MyInt;"
  "MyInt target = 0;";
EXPECT_EQ("typedef int MyInt;"
  "const MyInt target = 0;",
  runCheckOnCode(S));
```
I prefer the current version, especially with the bigger test later this file.



Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:83-84
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),

aaron.ballman wrote:
> This does what I would expect, but boy does it make me sad to see (because 
> `target` is not actually a `const int *` but is instead an `int * const`).
You are right, but I am not sure what the proper policy should be. I think the 
smarts to detect such potential improper const should be in the library code 
calling the transformation.
The utility should allow both transformations.



Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:1006
+}
+
+} // namespace test

aaron.ballman wrote:
> Can you also add some ObjC pointer tests?
i added the most basic test, do you think more is necessary? I don't know a lot 
about the objc languages.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

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

- address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395

Files:
  clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.h
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.h
  clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt

Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -7,6 +7,7 @@
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
+  AddConstTest.cpp
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
@@ -0,0 +1,1047 @@
+#include "../clang-tidy/utils/FixItHintUtils.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+using namespace clang::ast_matchers;
+using namespace utils::fixit;
+
+template 
+class ConstTransform : public ClangTidyCheck {
+public:
+  ConstTransform(StringRef CheckName, ClangTidyContext *Context)
+  : ClangTidyCheck(CheckName, Context) {}
+
+  void registerMatchers(MatchFinder *Finder) override {
+Finder->addMatcher(varDecl(hasName("target")).bind("var"), this);
+  }
+
+  void check(const MatchFinder::MatchResult ) override {
+const auto *D = Result.Nodes.getNodeAs("var");
+using utils::fixit::addQualifierToVarDecl;
+Optional Fix = addQualifierToVarDecl(
+*D, *Result.Context, DeclSpec::TQ::TQ_const, CT, CP);
+auto Diag = diag(D->getBeginLoc(), "doing const transformation");
+if (Fix)
+  Diag << *Fix;
+  }
+};
+} // namespace
+
+namespace test {
+using PointeeLTransform =
+ConstTransform;
+using PointeeRTransform =
+ConstTransform;
+
+using ValueLTransform =
+ConstTransform;
+using ValueRTransform =
+ConstTransform;
+
+// 
+// Test Value-like types. Everything with indirection is done later.
+// 
+
+TEST(Values, Builtin) {
+  StringRef Snippet = "int target = 0;";
+
+  EXPECT_EQ("const int target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("const int target = 0;",
+runCheckOnCode(Snippet));
+
+  EXPECT_EQ("int const target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("int const target = 0;",
+runCheckOnCode(Snippet));
+}
+TEST(Values, TypedefBuiltin) {
+  StringRef T = "typedef int MyInt;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, TypedefBuiltinPointer) {
+  StringRef T = "typedef int* MyInt;";
+  StringRef S = "MyInt target = nullptr;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltin) {
+  StringRef T = "using MyInt = int;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltinPointer) {
+  StringRef T = 

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Aaron Ballman via Phabricator via cfe-commits]

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

I think this generally looks good, thank you for all the hard work on this! I 
just found some minor nits and testing requests. Assuming no surprises with the 
tests, LGTM.




Comment at: 
clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp:175-176
 const SourceManager , Preprocessor *PP, Preprocessor *ModuleExpanderPP) 
{
-  Inserter = std::make_unique(SM, getLangOpts(),
-   IncludeStyle);
+  Inserter =
+  std::make_unique(SM, getLangOpts(), 
IncludeStyle);
   PP->addPPCallbacks(Inserter->CreatePPCallbacks());

Unrelated change?



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.h:37
+enum class QualifierTarget {
+  Pointee, /// Transforming a pointer goes for the pointee and not the pointer
+   /// itself. For references and normal values this option has no

aaron.ballman wrote:
> goes for -> attaches to?
attaches for -> attaches to



Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:66
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+

I don't think this is needed -- just add a newline between the literals and let 
string concat work its magic? (Same elsewhere)
```
StringRef S = "typedef int MyInt;"
  "MyInt target = 0;";
```



Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:83-84
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),

This does what I would expect, but boy does it make me sad to see (because 
`target` is not actually a `const int *` but is instead an `int * const`).



Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:535
+
+TEST(TagTypes, Struct) {
+  StringRef T = "struct Foo { int data; int method(); };\n";

Can you add a test for this scenario:
```
struct S {
  int i;
} x = { 0 };
```
where you want to apply the `const` to the type of `x`?



Comment at: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp:1006
+}
+
+} // namespace test

Can you also add some ObjC pointer tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

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

- clarify FIXME problem


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395

Files:
  clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.h
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.h
  clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt

Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -7,6 +7,7 @@
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
+  AddConstTest.cpp
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
@@ -0,0 +1,1009 @@
+#include "../clang-tidy/utils/FixItHintUtils.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+using namespace clang::ast_matchers;
+using namespace utils::fixit;
+
+template 
+class ConstTransform : public ClangTidyCheck {
+public:
+  ConstTransform(StringRef CheckName, ClangTidyContext *Context)
+  : ClangTidyCheck(CheckName, Context) {}
+
+  void registerMatchers(MatchFinder *Finder) override {
+Finder->addMatcher(varDecl(hasName("target")).bind("var"), this);
+  }
+
+  void check(const MatchFinder::MatchResult ) override {
+const auto *D = Result.Nodes.getNodeAs("var");
+using utils::fixit::addQualifierToVarDecl;
+Optional Fix = addQualifierToVarDecl(
+*D, *Result.Context, DeclSpec::TQ::TQ_const, CT, CP);
+auto Diag = diag(D->getBeginLoc(), "doing const transformation");
+if (Fix)
+  Diag << *Fix;
+  }
+};
+} // namespace
+
+namespace test {
+using PointeeLTransform =
+ConstTransform;
+using PointeeRTransform =
+ConstTransform;
+
+using ValueLTransform =
+ConstTransform;
+using ValueRTransform =
+ConstTransform;
+
+// 
+// Test Value-like types. Everything with indirection is done later.
+// 
+
+TEST(Values, Builtin) {
+  StringRef Snippet = "int target = 0;";
+
+  EXPECT_EQ("const int target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("const int target = 0;",
+runCheckOnCode(Snippet));
+
+  EXPECT_EQ("int const target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("int const target = 0;",
+runCheckOnCode(Snippet));
+}
+TEST(Values, TypedefBuiltin) {
+  StringRef T = "typedef int MyInt;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, TypedefBuiltinPointer) {
+  StringRef T = "typedef int* MyInt;";
+  StringRef S = "MyInt target = nullptr;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltin) {
+  StringRef T = "using MyInt = int;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltinPointer) {
+  StringRef T = "using MyInt = int*;";
+  StringRef S = "MyInt 

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

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

- adjust skipLParens code slightly to remove redundant checks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395

Files:
  clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.h
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.h
  clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt

Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -7,6 +7,7 @@
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
+  AddConstTest.cpp
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
@@ -0,0 +1,1008 @@
+#include "../clang-tidy/utils/FixItHintUtils.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+using namespace clang::ast_matchers;
+using namespace utils::fixit;
+
+template 
+class ConstTransform : public ClangTidyCheck {
+public:
+  ConstTransform(StringRef CheckName, ClangTidyContext *Context)
+  : ClangTidyCheck(CheckName, Context) {}
+
+  void registerMatchers(MatchFinder *Finder) override {
+Finder->addMatcher(varDecl(hasName("target")).bind("var"), this);
+  }
+
+  void check(const MatchFinder::MatchResult ) override {
+const auto *D = Result.Nodes.getNodeAs("var");
+using utils::fixit::addQualifierToVarDecl;
+Optional Fix = addQualifierToVarDecl(
+*D, *Result.Context, DeclSpec::TQ::TQ_const, CT, CP);
+auto Diag = diag(D->getBeginLoc(), "doing const transformation");
+if (Fix)
+  Diag << *Fix;
+  }
+};
+} // namespace
+
+namespace test {
+using PointeeLTransform =
+ConstTransform;
+using PointeeRTransform =
+ConstTransform;
+
+using ValueLTransform =
+ConstTransform;
+using ValueRTransform =
+ConstTransform;
+
+// 
+// Test Value-like types. Everything with indirection is done later.
+// 
+
+TEST(Values, Builtin) {
+  StringRef Snippet = "int target = 0;";
+
+  EXPECT_EQ("const int target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("const int target = 0;",
+runCheckOnCode(Snippet));
+
+  EXPECT_EQ("int const target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("int const target = 0;",
+runCheckOnCode(Snippet));
+}
+TEST(Values, TypedefBuiltin) {
+  StringRef T = "typedef int MyInt;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, TypedefBuiltinPointer) {
+  StringRef T = "typedef int* MyInt;";
+  StringRef S = "MyInt target = nullptr;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltin) {
+  StringRef T = "using MyInt = int;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltinPointer) {
+  StringRef T = "using MyInt 

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

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

- improve test situation and fix a crash coming from invalid source locations


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395

Files:
  clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.h
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.h
  clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt

Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -7,6 +7,7 @@
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
+  AddConstTest.cpp
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
@@ -0,0 +1,1008 @@
+#include "../clang-tidy/utils/FixItHintUtils.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+using namespace clang::ast_matchers;
+using namespace utils::fixit;
+
+template 
+class ConstTransform : public ClangTidyCheck {
+public:
+  ConstTransform(StringRef CheckName, ClangTidyContext *Context)
+  : ClangTidyCheck(CheckName, Context) {}
+
+  void registerMatchers(MatchFinder *Finder) override {
+Finder->addMatcher(varDecl(hasName("target")).bind("var"), this);
+  }
+
+  void check(const MatchFinder::MatchResult ) override {
+const auto *D = Result.Nodes.getNodeAs("var");
+using utils::fixit::addQualifierToVarDecl;
+Optional Fix = addQualifierToVarDecl(
+*D, *Result.Context, DeclSpec::TQ::TQ_const, CT, CP);
+auto Diag = diag(D->getBeginLoc(), "doing const transformation");
+if (Fix)
+  Diag << *Fix;
+  }
+};
+} // namespace
+
+namespace test {
+using PointeeLTransform =
+ConstTransform;
+using PointeeRTransform =
+ConstTransform;
+
+using ValueLTransform =
+ConstTransform;
+using ValueRTransform =
+ConstTransform;
+
+// 
+// Test Value-like types. Everything with indirection is done later.
+// 
+
+TEST(Values, Builtin) {
+  StringRef Snippet = "int target = 0;";
+
+  EXPECT_EQ("const int target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("const int target = 0;",
+runCheckOnCode(Snippet));
+
+  EXPECT_EQ("int const target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("int const target = 0;",
+runCheckOnCode(Snippet));
+}
+TEST(Values, TypedefBuiltin) {
+  StringRef T = "typedef int MyInt;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, TypedefBuiltinPointer) {
+  StringRef T = "typedef int* MyInt;";
+  StringRef S = "MyInt target = nullptr;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltin) {
+  StringRef T = "using MyInt = int;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltinPointer) {
+  StringRef 

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

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

- fix brace


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395

Files:
  clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.h
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.h
  clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt

Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -7,6 +7,7 @@
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
+  AddConstTest.cpp
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
@@ -0,0 +1,982 @@
+#include "../clang-tidy/utils/FixItHintUtils.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+using namespace clang::ast_matchers;
+using namespace utils::fixit;
+
+template 
+class ConstTransform : public ClangTidyCheck {
+public:
+  ConstTransform(StringRef CheckName, ClangTidyContext *Context)
+  : ClangTidyCheck(CheckName, Context) {}
+
+  void registerMatchers(MatchFinder *Finder) override {
+Finder->addMatcher(varDecl(hasName("target")).bind("var"), this);
+  }
+
+  void check(const MatchFinder::MatchResult ) override {
+const auto *D = Result.Nodes.getNodeAs("var");
+using utils::fixit::addQualifierToVarDecl;
+Optional Fix = addQualifierToVarDecl(
+*D, *Result.Context, DeclSpec::TQ::TQ_const, CT, CP);
+auto Diag = diag(D->getBeginLoc(), "doing const transformation");
+if (Fix)
+  Diag << *Fix;
+  }
+};
+} // namespace
+
+namespace test {
+using PointeeLTransform =
+ConstTransform;
+using PointeeRTransform =
+ConstTransform;
+
+using ValueLTransform =
+ConstTransform;
+using ValueRTransform =
+ConstTransform;
+
+// 
+// Test Value-like types. Everything with indirection is done later.
+// 
+
+TEST(Values, Builtin) {
+  StringRef Snippet = "int target = 0;";
+
+  EXPECT_EQ("const int target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("const int target = 0;",
+runCheckOnCode(Snippet));
+
+  EXPECT_EQ("int const target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("int const target = 0;",
+runCheckOnCode(Snippet));
+}
+TEST(Values, TypedefBuiltin) {
+  StringRef T = "typedef int MyInt;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, TypedefBuiltinPointer) {
+  StringRef T = "typedef int* MyInt;";
+  StringRef S = "MyInt target = nullptr;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltin) {
+  StringRef T = "using MyInt = int;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltinPointer) {
+  StringRef T = "using MyInt = int*;";
+  StringRef S = "MyInt target = 

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

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

- create fresh branch with latest diff of the revision
- fix review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395

Files:
  clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.h
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.h
  clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt

Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -7,6 +7,7 @@
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
+  AddConstTest.cpp
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
@@ -0,0 +1,982 @@
+#include "../clang-tidy/utils/FixItHintUtils.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+using namespace clang::ast_matchers;
+using namespace utils::fixit;
+
+template 
+class ConstTransform : public ClangTidyCheck {
+public:
+  ConstTransform(StringRef CheckName, ClangTidyContext *Context)
+  : ClangTidyCheck(CheckName, Context) {}
+
+  void registerMatchers(MatchFinder *Finder) override {
+Finder->addMatcher(varDecl(hasName("target")).bind("var"), this);
+  }
+
+  void check(const MatchFinder::MatchResult ) override {
+const auto *D = Result.Nodes.getNodeAs("var");
+using utils::fixit::addQualifierToVarDecl;
+Optional Fix = addQualifierToVarDecl(
+*D, *Result.Context, DeclSpec::TQ::TQ_const, CT, CP);
+auto Diag = diag(D->getBeginLoc(), "doing const transformation");
+if (Fix)
+  Diag << *Fix;
+  }
+};
+} // namespace
+
+namespace test {
+using PointeeLTransform =
+ConstTransform;
+using PointeeRTransform =
+ConstTransform;
+
+using ValueLTransform =
+ConstTransform;
+using ValueRTransform =
+ConstTransform;
+
+// 
+// Test Value-like types. Everything with indirection is done later.
+// 
+
+TEST(Values, Builtin) {
+  StringRef Snippet = "int target = 0;";
+
+  EXPECT_EQ("const int target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("const int target = 0;",
+runCheckOnCode(Snippet));
+
+  EXPECT_EQ("int const target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("int const target = 0;",
+runCheckOnCode(Snippet));
+}
+TEST(Values, TypedefBuiltin) {
+  StringRef T = "typedef int MyInt;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, TypedefBuiltinPointer) {
+  StringRef T = "typedef int* MyInt;";
+  StringRef S = "MyInt target = nullptr;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltin) {
+  StringRef T = "using MyInt = int;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:34-41
+static bool isArrayType(QualType QT) { return isa(QT.getTypePtr()); 
}
+static bool isReferenceType(QualType QT) {
+  return isa(QT.getTypePtr());
+}
+static bool isPointerType(const Type *T) { return isa(T); }
+static bool isPointerType(QualType QT) {
+  return isPointerType(QT.getTypePtr());

aaron.ballman wrote:
> I don't see how these are improvements over checking `QT->isArrayType()` and 
> friends within the caller.
True!



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:82
+return (llvm::Twine(' ') + DeclSpec::getSpecifierName(Qualifier)).str();
+  return (llvm::Twine(DeclSpec::getSpecifierName(Qualifier)) + llvm::Twine(' 
'))
+  .str();

aaron.ballman wrote:
> Do you need both casts for `Twine`? I would imagine only the first is needed.
In case of `' '` it is necessary, in case of `" "` its not. I go with `" "`, 
should not be more expensive or anything, right?



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:166
+
+  llvm_unreachable("All paths should have been handled");
+}

aaron.ballman wrote:
> I think this path is reachable -- it should be an `assert()` instead and 
> return `None`.
When testing LLVM i saw some path triggering the assertions and unreachables.

I want to go the `None`-route for now and only emit warnings without fixit. 
That will help to find false-positives unhandled cases better and is less 
problematic.

Is it only objc-code that could trigger this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:30-31
+static bool isValueType(const Type *T) {
+  return !(isa(T) || isa(T) || isa(T) ||
+   isa(T));
 }

ObjC pointer types?



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:34-41
+static bool isArrayType(QualType QT) { return isa(QT.getTypePtr()); 
}
+static bool isReferenceType(QualType QT) {
+  return isa(QT.getTypePtr());
+}
+static bool isPointerType(const Type *T) { return isa(T); }
+static bool isPointerType(QualType QT) {
+  return isPointerType(QT.getTypePtr());

I don't see how these are improvements over checking `QT->isArrayType()` and 
friends within the caller.



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:82
+return (llvm::Twine(' ') + DeclSpec::getSpecifierName(Qualifier)).str();
+  return (llvm::Twine(DeclSpec::getSpecifierName(Qualifier)) + llvm::Twine(' 
'))
+  .str();

Do you need both casts for `Twine`? I would imagine only the first is needed.



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:166
+
+  llvm_unreachable("All paths should have been handled");
+}

I think this path is reachable -- it should be an `assert()` instead and return 
`None`.



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:192
+  QualifierPolicy QualPolicy,
+  const ASTContext *Context) {
+  assert((QualPolicy == QualifierPolicy::Left ||

I think `Context` should be by reference instead of by pointer.



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp:228-229
+
+  llvm_unreachable(
+  "All possible combinations should have been handled already");
+}

I think this is reachable as well.



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.h:27
 
+/// This enum defines where the 'const' shall be preferably added.
+enum class QualifierPolicy {

const -> qualifier (similar for the comments in the enumerators)



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.h:33
+
+/// This enum defines which entity is the target for adding the 'const'. This
+/// makes only a difference for pointer-types. Other types behave identical

Same here.



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.h:37
+enum class QualifierTarget {
+  Pointee, /// Transforming a pointer goes for the pointee and not the pointer
+   /// itself. For references and normal values this option has no

goes for -> attaches to?



Comment at: clang-tools-extra/clang-tidy/utils/FixItHintUtils.h:45
+
+/// \brief Creates fix to make ``VarDecl`` const qualified. Only valid if
+/// `Var` is isolated in written code. `int foo = 42;`

Comment seems out of date here as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

I know it has been a long time, but this patch is ready, i think :)
i would appreciate some reviews ;)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth updated this revision to Diff 229705.
JonasToth marked 2 inline comments as done.
JonasToth removed a subscriber: mgehre.
JonasToth added a comment.

- [Misc] port patch to monorepo
- add more test cases for the transformation


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395

Files:
  clang-tools-extra/clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tools-extra/clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.cpp
  clang-tools-extra/clang-tidy/utils/FixItHintUtils.h
  clang-tools-extra/clang-tidy/utils/LexerUtils.cpp
  clang-tools-extra/clang-tidy/utils/LexerUtils.h
  clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
  clang-tools-extra/unittests/clang-tidy/CMakeLists.txt

Index: clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
===
--- clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
+++ clang-tools-extra/unittests/clang-tidy/CMakeLists.txt
@@ -7,6 +7,7 @@
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
+  AddConstTest.cpp
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
===
--- /dev/null
+++ clang-tools-extra/unittests/clang-tidy/AddConstTest.cpp
@@ -0,0 +1,982 @@
+#include "../clang-tidy/utils/FixItHintUtils.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+using namespace clang::ast_matchers;
+using namespace utils::fixit;
+
+template 
+class ConstTransform : public ClangTidyCheck {
+public:
+  ConstTransform(StringRef CheckName, ClangTidyContext *Context)
+  : ClangTidyCheck(CheckName, Context) {}
+
+  void registerMatchers(MatchFinder *Finder) override {
+Finder->addMatcher(varDecl(hasName("target")).bind("var"), this);
+  }
+
+  void check(const MatchFinder::MatchResult ) override {
+const auto *D = Result.Nodes.getNodeAs("var");
+using utils::fixit::addQualifierToVarDecl;
+Optional Fix = addQualifierToVarDecl(*D, DeclSpec::TQ::TQ_const,
+CT, CP, Result.Context);
+auto Diag = diag(D->getBeginLoc(), "doing const transformation");
+if (Fix)
+  Diag << *Fix;
+  }
+};
+} // namespace
+
+namespace test {
+using PointeeLTransform =
+ConstTransform;
+using PointeeRTransform =
+ConstTransform;
+
+using ValueLTransform =
+ConstTransform;
+using ValueRTransform =
+ConstTransform;
+
+// 
+// Test Value-like types. Everything with indirection is done later.
+// 
+
+TEST(Values, Builtin) {
+  StringRef Snippet = "int target = 0;";
+
+  EXPECT_EQ("const int target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("const int target = 0;",
+runCheckOnCode(Snippet));
+
+  EXPECT_EQ("int const target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("int const target = 0;",
+runCheckOnCode(Snippet));
+}
+TEST(Values, TypedefBuiltin) {
+  StringRef T = "typedef int MyInt;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, TypedefBuiltinPointer) {
+  StringRef T = "typedef int* MyInt;";
+  StringRef S = "MyInt target = nullptr;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, UsingBuiltin) {
+  StringRef T = "using MyInt = int;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth marked 5 inline comments as done.
JonasToth added inline comments.
Herald added a subscriber: mgehre.



Comment at: clang-tidy/utils/FixItHintUtils.cpp:35
+static bool isValueType(QualType QT) { return isValueType(QT.getTypePtr()); }
+static bool isArrayType(QualType QT) { return isa(QT.getTypePtr()); 
}
+static bool isReferenceType(QualType QT) {

lebedev.ri wrote:
> JonasToth wrote:
> > aaron.ballman wrote:
> > > This file is replicating a bunch of functionality that exists on the 
> > > `Type*` already. Why do this rather than have the caller do 
> > > `QT->isArrayType()` and skip this function entirely? (Same exists 
> > > elsewhere here.)
> > I had the issue that typedefs are resolved, but shouldn't. If I am not 
> > mistaken `QT->isArrayType()` would go to the canconical type.
> > 
> > ```
> > using MyPtr = int*;
> > MyPtr foo = nullptr;
> > ```
> > Is treated wrong in that case.
> There is a test for that, right?
Yes, both `typedef` and `using`.



Comment at: unittests/clang-tidy/AddConstTest.cpp:733
+  StringRef T = "template  void f(T v) \n";
+  StringRef S = "{ T target = v; }";
+  auto Cat = [](StringRef S) { return (T + S).str(); };

lebedev.ri wrote:
> lebedev.ri wrote:
> > JonasToth wrote:
> > > alexfh wrote:
> > > > It would be interesting to see test cases with multiple instantiations 
> > > > of the template the fix applies to.
> > > I added test for a template function with many instantiations, but there 
> > > should not be a difference between the instantiations, because only the 
> > > original code would be transformed, and there the 'how it looks' counts, 
> > > so in this case it will be treated as a value.
> > > Did I misinterpret your question?
> > How about 
> > https://en.cppreference.com/w/cpp/language/function_template#Explicit_instantiation
> >  ?
> > I don't see any tests for that.
> Also, is there any test coverage missing for 
> https://en.cppreference.com/w/cpp/language/template_specialization
> and
> https://en.cppreference.com/w/cpp/language/partial_specialization
> ?
i added tests for specializations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.



Comment at: unittests/clang-tidy/AddConstTest.cpp:733
+  StringRef T = "template  void f(T v) \n";
+  StringRef S = "{ T target = v; }";
+  auto Cat = [](StringRef S) { return (T + S).str(); };

lebedev.ri wrote:
> JonasToth wrote:
> > alexfh wrote:
> > > It would be interesting to see test cases with multiple instantiations of 
> > > the template the fix applies to.
> > I added test for a template function with many instantiations, but there 
> > should not be a difference between the instantiations, because only the 
> > original code would be transformed, and there the 'how it looks' counts, so 
> > in this case it will be treated as a value.
> > Did I misinterpret your question?
> How about 
> https://en.cppreference.com/w/cpp/language/function_template#Explicit_instantiation
>  ?
> I don't see any tests for that.
Also, is there any test coverage missing for 
https://en.cppreference.com/w/cpp/language/template_specialization
and
https://en.cppreference.com/w/cpp/language/partial_specialization
?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

I think this looks good now, one nit about test coverage.
Did you run this through your buildbot, any issues?




Comment at: clang-tidy/utils/FixItHintUtils.cpp:35
+static bool isValueType(QualType QT) { return isValueType(QT.getTypePtr()); }
+static bool isArrayType(QualType QT) { return isa(QT.getTypePtr()); 
}
+static bool isReferenceType(QualType QT) {

JonasToth wrote:
> aaron.ballman wrote:
> > This file is replicating a bunch of functionality that exists on the 
> > `Type*` already. Why do this rather than have the caller do 
> > `QT->isArrayType()` and skip this function entirely? (Same exists elsewhere 
> > here.)
> I had the issue that typedefs are resolved, but shouldn't. If I am not 
> mistaken `QT->isArrayType()` would go to the canconical type.
> 
> ```
> using MyPtr = int*;
> MyPtr foo = nullptr;
> ```
> Is treated wrong in that case.
There is a test for that, right?



Comment at: unittests/clang-tidy/AddConstTest.cpp:733
+  StringRef T = "template  void f(T v) \n";
+  StringRef S = "{ T target = v; }";
+  auto Cat = [](StringRef S) { return (T + S).str(); };

JonasToth wrote:
> alexfh wrote:
> > It would be interesting to see test cases with multiple instantiations of 
> > the template the fix applies to.
> I added test for a template function with many instantiations, but there 
> should not be a difference between the instantiations, because only the 
> original code would be transformed, and there the 'how it looks' counts, so 
> in this case it will be treated as a value.
> Did I misinterpret your question?
How about 
https://en.cppreference.com/w/cpp/language/function_template#Explicit_instantiation
 ?
I don't see any tests for that.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

ping :)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

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



Comment at: unittests/clang-tidy/AddConstTest.cpp:15
+
+template 

alexfh wrote:
> What's the point of default values of template arguments here?
Wups, relict of older version.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

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

- address review comments
- rebase to master


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395

Files:
  clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tidy/utils/FixItHintUtils.cpp
  clang-tidy/utils/FixItHintUtils.h
  clang-tidy/utils/LexerUtils.cpp
  clang-tidy/utils/LexerUtils.h
  unittests/clang-tidy/AddConstTest.cpp
  unittests/clang-tidy/CMakeLists.txt

Index: unittests/clang-tidy/CMakeLists.txt
===
--- unittests/clang-tidy/CMakeLists.txt
+++ unittests/clang-tidy/CMakeLists.txt
@@ -7,6 +7,7 @@
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
+  AddConstTest.cpp
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
Index: unittests/clang-tidy/AddConstTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/AddConstTest.cpp
@@ -0,0 +1,905 @@
+#include "../clang-tidy/utils/FixItHintUtils.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+using namespace clang::ast_matchers;
+using namespace utils::fixit;
+
+template 
+class ConstTransform : public ClangTidyCheck {
+public:
+  ConstTransform(StringRef CheckName, ClangTidyContext *Context)
+  : ClangTidyCheck(CheckName, Context) {}
+
+  void registerMatchers(MatchFinder *Finder) override {
+Finder->addMatcher(varDecl(hasName("target")).bind("var"), this);
+  }
+
+  void check(const MatchFinder::MatchResult ) override {
+const auto *D = Result.Nodes.getNodeAs("var");
+using utils::fixit::addQualifierToVarDecl;
+Optional Fix = addQualifierToVarDecl(*D, DeclSpec::TQ::TQ_const,
+CT, CP, Result.Context);
+auto Diag = diag(D->getBeginLoc(), "doing const transformation");
+if (Fix)
+  Diag << *Fix;
+  }
+};
+} // namespace
+
+namespace test {
+using PointeeLTransform =
+ConstTransform;
+using PointeeRTransform =
+ConstTransform;
+
+using ValueLTransform =
+ConstTransform;
+using ValueRTransform =
+ConstTransform;
+
+// 
+// Test Value-like types. Everything with indirection is done later.
+// 
+
+TEST(Values, Builtin) {
+  StringRef Snippet = "int target = 0;";
+
+  EXPECT_EQ("const int target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("const int target = 0;",
+runCheckOnCode(Snippet));
+
+  EXPECT_EQ("int const target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("int const target = 0;",
+runCheckOnCode(Snippet));
+}
+TEST(Values, TypedefBuiltin) {
+  StringRef T = "typedef int MyInt;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, TypedefBuiltinPointer) {
+  StringRef T = "typedef int* MyInt;";
+  StringRef S = "MyInt target = nullptr;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, AutoValue) {
+  StringRef T = "int f() { return 42; }\n";
+  StringRef S = "auto target = f();";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const auto target = f();"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const auto target = f();"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("auto const target = f();"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("auto const target = f();"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, AutoPointer) {
+  StringRef T = "int* f() { return nullptr; }\n";
+  StringRef S = "auto target = f();";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const auto target = f();"),
+

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

A few more comments.




Comment at: clang-tidy/utils/FixItHintUtils.cpp:83
+return (llvm::Twine(' ') +
+llvm::Twine(DeclSpec::getSpecifierName(Qualifier)))
+.str();

The first operand being llvm::Twine is enough for the whole chained 
concatenation expression to be llvm::Twine.



Comment at: unittests/clang-tidy/AddConstTest.cpp:15
+
+template 

What's the point of default values of template arguments here?



Comment at: unittests/clang-tidy/AddConstTest.cpp:53
+
+// TODO: Template-code
+

Is this still needed? If yes, please use "FIXME" instead and expand on what 
exactly is missing or needs to be done.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

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

another ping. @alexfh and @aaron.ballman you commented on prior versions. Would 
be nice if you could take a (final) look at this patch!


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.
Herald added a project: clang.

ping


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

ping.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

ping :)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

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

- generalize to DeclSpec::TQ
- add dependent type-tests with `typename`


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395

Files:
  clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tidy/utils/FixItHintUtils.cpp
  clang-tidy/utils/FixItHintUtils.h
  clang-tidy/utils/LexerUtils.cpp
  clang-tidy/utils/LexerUtils.h
  unittests/clang-tidy/AddConstTest.cpp
  unittests/clang-tidy/CMakeLists.txt

Index: unittests/clang-tidy/CMakeLists.txt
===
--- unittests/clang-tidy/CMakeLists.txt
+++ unittests/clang-tidy/CMakeLists.txt
@@ -7,6 +7,7 @@
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
+  AddConstTest.cpp
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
Index: unittests/clang-tidy/AddConstTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/AddConstTest.cpp
@@ -0,0 +1,908 @@
+#include "../clang-tidy/utils/FixItHintUtils.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+using namespace clang::ast_matchers;
+using namespace utils::fixit;
+
+template 
+class ConstTransform : public ClangTidyCheck {
+public:
+  ConstTransform(StringRef CheckName, ClangTidyContext *Context)
+  : ClangTidyCheck(CheckName, Context) {}
+
+  void registerMatchers(MatchFinder *Finder) override {
+Finder->addMatcher(varDecl(hasName("target")).bind("var"), this);
+  }
+
+  void check(const MatchFinder::MatchResult ) override {
+const auto *D = Result.Nodes.getNodeAs("var");
+using utils::fixit::addQualifierToVarDecl;
+Optional Fix = addQualifierToVarDecl(*D, DeclSpec::TQ::TQ_const,
+CT, CP, Result.Context);
+auto Diag = diag(D->getBeginLoc(), "doing const transformation");
+if (Fix)
+  Diag << *Fix;
+  }
+};
+} // namespace
+
+namespace test {
+using PointeeLTransform =
+ConstTransform;
+using PointeeRTransform =
+ConstTransform;
+
+using ValueLTransform =
+ConstTransform;
+using ValueRTransform =
+ConstTransform;
+
+// 
+// Test Value-like types. Everything with indirection is done later.
+// 
+
+// TODO: Template-code
+
+TEST(Values, Builtin) {
+  StringRef Snippet = "int target = 0;";
+
+  EXPECT_EQ("const int target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("const int target = 0;",
+runCheckOnCode(Snippet));
+
+  EXPECT_EQ("int const target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("int const target = 0;",
+runCheckOnCode(Snippet));
+}
+TEST(Values, TypedefBuiltin) {
+  StringRef T = "typedef int MyInt;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, TypedefBuiltinPointer) {
+  StringRef T = "typedef int* MyInt;";
+  StringRef S = "MyInt target = nullptr;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, AutoValue) {
+  StringRef T = "int f() { return 42; }\n";
+  StringRef S = "auto target = f();";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const auto target = f();"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const auto target = f();"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("auto const target = f();"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("auto const target = f();"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, AutoPointer) {
+  StringRef T = "int* f() { return nullptr; }\n";
+  StringRef S = "auto target = f();";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const auto target = f();"),
+  

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

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



Comment at: clang-tidy/utils/FixItHintUtils.cpp:35
+static bool isValueType(QualType QT) { return isValueType(QT.getTypePtr()); }
+static bool isArrayType(QualType QT) { return isa(QT.getTypePtr()); 
}
+static bool isReferenceType(QualType QT) {

aaron.ballman wrote:
> This file is replicating a bunch of functionality that exists on the `Type*` 
> already. Why do this rather than have the caller do `QT->isArrayType()` and 
> skip this function entirely? (Same exists elsewhere here.)
I had the issue that typedefs are resolved, but shouldn't. If I am not mistaken 
`QT->isArrayType()` would go to the canconical type.

```
using MyPtr = int*;
MyPtr foo = nullptr;
```
Is treated wrong in that case.



Comment at: clang-tidy/utils/FixItHintUtils.h:47
+/// \c SourceLocation it is not returned.
+Optional changeVarDeclToConst(const VarDecl ,
+ ConstTarget CT = ConstTarget::Pointee,

aaron.ballman wrote:
> I feel like this could be trivially generalized to any type qualifier, not 
> just `const`. How would you feel about renaming this to: 
> `addQualifierToVarDecl()` and adding a parameter of type `DeclSpec::TQ` to 
> specify which qualifier to add?
Yes, sounds good.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/utils/FixItHintUtils.cpp:35
+static bool isValueType(QualType QT) { return isValueType(QT.getTypePtr()); }
+static bool isArrayType(QualType QT) { return isa(QT.getTypePtr()); 
}
+static bool isReferenceType(QualType QT) {

This file is replicating a bunch of functionality that exists on the `Type*` 
already. Why do this rather than have the caller do `QT->isArrayType()` and 
skip this function entirely? (Same exists elsewhere here.)



Comment at: clang-tidy/utils/FixItHintUtils.cpp:109
+ const ASTContext ) {
+  // The pointer itself shall be marked as `const`. This is always right
+  // of the '*' or in front of the identifier.

always right -> always to the right



Comment at: clang-tidy/utils/FixItHintUtils.h:47
+/// \c SourceLocation it is not returned.
+Optional changeVarDeclToConst(const VarDecl ,
+ ConstTarget CT = ConstTarget::Pointee,

I feel like this could be trivially generalized to any type qualifier, not just 
`const`. How would you feel about renaming this to: `addQualifierToVarDecl()` 
and adding a parameter of type `DeclSpec::TQ` to specify which qualifier to add?


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

In D54395#1309423 , @lebedev.ri wrote:

> Looks reasonable to me (or at least all these tests make this look good :)), 
> but i'm not sure i can fully review this.


No problem ;)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

Looks reasonable to me (or at least all these tests make this look good :)), 
but i'm not sure i can fully review this.


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

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

reabse to master + ping :)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D54395

Files:
  clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tidy/utils/FixItHintUtils.cpp
  clang-tidy/utils/FixItHintUtils.h
  clang-tidy/utils/LexerUtils.cpp
  clang-tidy/utils/LexerUtils.h
  unittests/clang-tidy/AddConstTest.cpp
  unittests/clang-tidy/CMakeLists.txt

Index: unittests/clang-tidy/CMakeLists.txt
===
--- unittests/clang-tidy/CMakeLists.txt
+++ unittests/clang-tidy/CMakeLists.txt
@@ -7,6 +7,7 @@
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
+  AddConstTest.cpp
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
Index: unittests/clang-tidy/AddConstTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/AddConstTest.cpp
@@ -0,0 +1,857 @@
+#include "../clang-tidy/utils/FixItHintUtils.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+using namespace clang::ast_matchers;
+using namespace utils::fixit;
+
+template 
+class ConstTransform : public ClangTidyCheck {
+public:
+  ConstTransform(StringRef CheckName, ClangTidyContext *Context)
+  : ClangTidyCheck(CheckName, Context) {}
+
+  void registerMatchers(MatchFinder *Finder) override {
+Finder->addMatcher(varDecl(hasName("target")).bind("var"), this);
+  }
+
+  void check(const MatchFinder::MatchResult ) override {
+const auto *D = Result.Nodes.getNodeAs("var");
+using utils::fixit::changeVarDeclToConst;
+Optional Fix = changeVarDeclToConst(*D, CT, CP, Result.Context);
+auto Diag = diag(D->getBeginLoc(), "doing const transformation");
+if (Fix)
+  Diag << *Fix;
+  }
+};
+} // namespace
+
+namespace test {
+using PointeeLTransform =
+ConstTransform;
+using PointeeRTransform =
+ConstTransform;
+
+using ValueLTransform = ConstTransform;
+using ValueRTransform = ConstTransform;
+
+// 
+// Test Value-like types. Everything with indirection is done later.
+// 
+
+// TODO: Template-code
+
+TEST(Values, Builtin) {
+  StringRef Snippet = "int target = 0;";
+
+  EXPECT_EQ("const int target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("const int target = 0;",
+runCheckOnCode(Snippet));
+
+  EXPECT_EQ("int const target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("int const target = 0;",
+runCheckOnCode(Snippet));
+}
+TEST(Values, TypedefBuiltin) {
+  StringRef T = "typedef int MyInt;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, TypedefBuiltinPointer) {
+  StringRef T = "typedef int* MyInt;";
+  StringRef S = "MyInt target = nullptr;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, AutoValue) {
+  StringRef T = "int f() { return 42; }\n";
+  StringRef S = "auto target = f();";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const auto target = f();"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const auto target = f();"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("auto const target = f();"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("auto const target = f();"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, AutoPointer) {
+  StringRef T = "int* f() { return nullptr; }\n";
+  StringRef S = "auto target = f();";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const auto target = f();"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const auto target = f();"),
+runCheckOnCode(Cat(S)));
+
+  

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: unittests/clang-tidy/AddConstTest.cpp:733
+  StringRef T = "template  void f(T v) \n";
+  StringRef S = "{ T target = v; }";
+  auto Cat = [](StringRef S) { return (T + S).str(); };

alexfh wrote:
> It would be interesting to see test cases with multiple instantiations of the 
> template the fix applies to.
I added test for a template function with many instantiations, but there should 
not be a difference between the instantiations, because only the original code 
would be transformed, and there the 'how it looks' counts, so in this case it 
will be treated as a value.
Did I misinterpret your question?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

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

add tests for different template instantiations


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54395

Files:
  clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tidy/utils/FixItHintUtils.cpp
  clang-tidy/utils/FixItHintUtils.h
  clang-tidy/utils/LexerUtils.cpp
  clang-tidy/utils/LexerUtils.h
  unittests/clang-tidy/AddConstTest.cpp
  unittests/clang-tidy/CMakeLists.txt

Index: unittests/clang-tidy/CMakeLists.txt
===
--- unittests/clang-tidy/CMakeLists.txt
+++ unittests/clang-tidy/CMakeLists.txt
@@ -7,6 +7,7 @@
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
+  AddConstTest.cpp
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
Index: unittests/clang-tidy/AddConstTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/AddConstTest.cpp
@@ -0,0 +1,857 @@
+#include "../clang-tidy/utils/FixItHintUtils.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+using namespace clang::ast_matchers;
+using namespace utils::fixit;
+
+template 
+class ConstTransform : public ClangTidyCheck {
+public:
+  ConstTransform(StringRef CheckName, ClangTidyContext *Context)
+  : ClangTidyCheck(CheckName, Context) {}
+
+  void registerMatchers(MatchFinder *Finder) override {
+Finder->addMatcher(varDecl(hasName("target")).bind("var"), this);
+  }
+
+  void check(const MatchFinder::MatchResult ) override {
+const auto *D = Result.Nodes.getNodeAs("var");
+using utils::fixit::changeVarDeclToConst;
+Optional Fix = changeVarDeclToConst(*D, CT, CP, Result.Context);
+auto Diag = diag(D->getBeginLoc(), "doing const transformation");
+if (Fix)
+  Diag << *Fix;
+  }
+};
+} // namespace
+
+namespace test {
+using PointeeLTransform =
+ConstTransform;
+using PointeeRTransform =
+ConstTransform;
+
+using ValueLTransform = ConstTransform;
+using ValueRTransform = ConstTransform;
+
+// 
+// Test Value-like types. Everything with indirection is done later.
+// 
+
+// TODO: Template-code
+
+TEST(Values, Builtin) {
+  StringRef Snippet = "int target = 0;";
+
+  EXPECT_EQ("const int target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("const int target = 0;",
+runCheckOnCode(Snippet));
+
+  EXPECT_EQ("int const target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("int const target = 0;",
+runCheckOnCode(Snippet));
+}
+TEST(Values, TypedefBuiltin) {
+  StringRef T = "typedef int MyInt;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, TypedefBuiltinPointer) {
+  StringRef T = "typedef int* MyInt;";
+  StringRef S = "MyInt target = nullptr;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, AutoValue) {
+  StringRef T = "int f() { return 42; }\n";
+  StringRef S = "auto target = f();";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const auto target = f();"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const auto target = f();"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("auto const target = f();"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("auto const target = f();"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, AutoPointer) {
+  StringRef T = "int* f() { return nullptr; }\n";
+  StringRef S = "auto target = f();";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const auto target = f();"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const auto target = f();"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("auto const target = f();"),
+

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: unittests/clang-tidy/AddConstTest.cpp:733
+  StringRef T = "template  void f(T v) \n";
+  StringRef S = "{ T target = v; }";
+  auto Cat = [](StringRef S) { return (T + S).str(); };

It would be interesting to see test cases with multiple instantiations of the 
template the fix applies to.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54395



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

[PATCH] D54395: [clang-tidy] implement utility-function to add 'const' to variables

[Jonas Toth via Phabricator via cfe-commits]

JonasToth created this revision.
JonasToth added reviewers: aaron.ballman, hokein, alexfh, shuaiwang, lebedev.ri.
Herald added subscribers: cfe-commits, xazax.hun, mgorny.
JonasToth added a dependent revision: D45444: [clang-tidy] implement new check 
for const-correctness.

This patch extends the already existing facility to add 'const' to variables
to be more flexible and correct. The previous version did not consider pointers
as value AND pointee. For future automatic introduction for const-correctness
this shortcoming needs to be fixed.
It always allows configuration where the 'const' token is inserted, either on
the left side (if possible) or the right side.
It adds many unit-tests to the utility-function that did not exist before, as
the function was implicitly tested through clang-tidy checks. These
tests were not changed, as the API is still compatible.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54395

Files:
  clang-tidy/performance/ForRangeCopyCheck.cpp
  clang-tidy/performance/UnnecessaryCopyInitialization.cpp
  clang-tidy/performance/UnnecessaryValueParamCheck.cpp
  clang-tidy/utils/FixItHintUtils.cpp
  clang-tidy/utils/FixItHintUtils.h
  clang-tidy/utils/LexerUtils.cpp
  clang-tidy/utils/LexerUtils.h
  unittests/clang-tidy/AddConstTest.cpp
  unittests/clang-tidy/CMakeLists.txt

Index: unittests/clang-tidy/CMakeLists.txt
===
--- unittests/clang-tidy/CMakeLists.txt
+++ unittests/clang-tidy/CMakeLists.txt
@@ -7,6 +7,7 @@
 include_directories(${CLANG_LINT_SOURCE_DIR})
 
 add_extra_unittest(ClangTidyTests
+  AddConstTest.cpp
   ClangTidyDiagnosticConsumerTest.cpp
   ClangTidyOptionsTest.cpp
   IncludeInserterTest.cpp
Index: unittests/clang-tidy/AddConstTest.cpp
===
--- /dev/null
+++ unittests/clang-tidy/AddConstTest.cpp
@@ -0,0 +1,826 @@
+#include "../clang-tidy/utils/FixItHintUtils.h"
+#include "ClangTidyTest.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Tooling/Tooling.h"
+#include "gtest/gtest.h"
+
+namespace clang {
+namespace tidy {
+
+namespace {
+using namespace clang::ast_matchers;
+using namespace utils::fixit;
+
+template 
+class ConstTransform : public ClangTidyCheck {
+public:
+  ConstTransform(StringRef CheckName, ClangTidyContext *Context)
+  : ClangTidyCheck(CheckName, Context) {}
+
+  void registerMatchers(MatchFinder *Finder) override {
+Finder->addMatcher(varDecl(hasName("target")).bind("var"), this);
+  }
+
+  void check(const MatchFinder::MatchResult ) override {
+const auto *D = Result.Nodes.getNodeAs("var");
+using utils::fixit::changeVarDeclToConst;
+Optional Fix = changeVarDeclToConst(*D, CT, CP, Result.Context);
+auto Diag = diag(D->getBeginLoc(), "doing const transformation");
+if (Fix)
+  Diag << *Fix;
+  }
+};
+} // namespace
+
+namespace test {
+using PointeeLTransform =
+ConstTransform;
+using PointeeRTransform =
+ConstTransform;
+
+using ValueLTransform = ConstTransform;
+using ValueRTransform = ConstTransform;
+
+// 
+// Test Value-like types. Everything with indirection is done later.
+// 
+
+// TODO: Template-code
+
+TEST(Values, Builtin) {
+  StringRef Snippet = "int target = 0;";
+
+  EXPECT_EQ("const int target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("const int target = 0;",
+runCheckOnCode(Snippet));
+
+  EXPECT_EQ("int const target = 0;", runCheckOnCode(Snippet));
+  EXPECT_EQ("int const target = 0;",
+runCheckOnCode(Snippet));
+}
+TEST(Values, TypedefBuiltin) {
+  StringRef T = "typedef int MyInt;";
+  StringRef S = "MyInt target = 0;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = 0;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = 0;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, TypedefBuiltinPointer) {
+  StringRef T = "typedef int* MyInt;";
+  StringRef S = "MyInt target = nullptr;";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("const MyInt target = nullptr;"),
+runCheckOnCode(Cat(S)));
+
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+  EXPECT_EQ(Cat("MyInt const target = nullptr;"),
+runCheckOnCode(Cat(S)));
+}
+TEST(Values, AutoValue) {
+  StringRef T = "int f() { return 42; }\n";
+  StringRef S = "auto target = f();";
+  auto Cat = [](StringRef S) { return (T + S).str(); };
+
+