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