[PATCH] D62977: [clang-tidy]: Google: new check 'google-upgrade-googletest-case'

[Eric Fiselier via Phabricator via cfe-commits]

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