EricWF added a comment.
Generally this LGTM.
I'll take another pass after the comments are addressed.
================
Comment at:
clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp:19
+
+static const llvm::StringRef CheckMessage =
+ "Googletest APIs named with 'case' are deprecated; use equivalent APIs "
----------------
Could you rename this to better represent what the diagnostic text is saying,
rather than just being diagnostic text?
I think it'll help readability below.
Why not put this inside the unnamed namespace instead?
Same with the function below.
================
Comment at:
clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp:201
+ const MatchFinder::MatchResult &Result) {
+ internal::Matcher<NodeType> IsInsideTemplate =
+ hasAncestor(decl(anyOf(classTemplateDecl(), functionTemplateDecl())));
----------------
Instead of walking the tree, can't you ask if the Node is dependent in some way?
================
Comment at:
clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp:294
+ } else {
+ // This is a match for `TestCase` to `TestSuite` refactoring.
+ ReplacementText = "TestSuite";
----------------
Maybe add an assertion for this comment?
================
Comment at: clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.h:23
+/// For the user-facing documentation see:
+///
http://clang.llvm.org/extra/clang-tidy/checks/google-upgrade-googletest-case.html
+class UpgradeGoogletestCaseCheck : public ClangTidyCheck {
----------------
`https` all the things!
================
Comment at:
clang-tools-extra/test/clang-tidy/google-upgrade-googletest-case.cpp:9
+TYPED_TEST_CASE(FooTest, FooTypes);
+// CHECK-MESSAGES: [[@LINE-1]]:1: warning: Googletest APIs named with 'case'
are deprecated; use equivalent APIs named with 'suite'
+// CHECK-FIXES: TYPED_TEST_SUITE(FooTest, FooTypes);
----------------
You probably don't have to test for the full message every time. but I don't
know it matters either way.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62977/new/
https://reviews.llvm.org/D62977
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
