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