[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 &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) {
-  Inserter = std::make_unique<utils::IncludeInserter>(SM, getLangOpts(),
-                                                       IncludeStyle);
+  Inserter =
+      std::make_unique<utils::IncludeInserter>(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 = [&T](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<ValueLTransform>(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