[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 = [&T](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<ValueLTransform>(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<ValueLTransform>(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