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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
