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

[Alex Strelnikov via Phabricator via cfe-commits]

astrelni marked 13 inline comments as done.
astrelni added a comment.

In D62977#1556346 <https://reviews.llvm.org/D62977#1556346>, @lebedev.ri wrote:

> In D62977#1540184 <https://reviews.llvm.org/D62977#1540184>, @lebedev.ri wrote:
>
> > Without seeing the tests - what version checks does this have?
> >  It shouldn't fire if the googletest version is the one before that rename.
>
>
> I don't believe this question was answered.


Sorry, the original comment got lost between me updating two patches as I noticed I did it wrong without seeing your comment.

Unfortunately there are no versioning macros in googletest, so I'm not sure how to keep this check from providing warnings if it is run while depending on a version that is too early. The new names are in master and will be part of an upcoming version 1.9 release. I have tried to update the documentation to clarify which version of googletest this intended for. Please let me know how you think we should proceed.



================
Comment at: clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp:30
 #include "UnnamedNamespaceInHeaderCheck.h"
 #include "UsingNamespaceDirectiveCheck.h"
 
----------------
EricWF wrote:
> Just tried building this. I think you're missing an include for `UpgradeGoogletestCaseCheck`.
Thanks for catching this, was having issues in the diff.


================
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 "
----------------
EricWF wrote:
> 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.
I had a previous comment that llvm coding guidelines prefer static and like to keep anonymous namespaces as small as possible and only around type declarations.


================
Comment at: clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp:20
+static const llvm::StringRef CheckMessage =
+    "Googletest APIs named with 'case' are deprecated; use equivalent APIs "
+    "named with 'suite'";
----------------
aaron.ballman wrote:
> Should this be spelled `Google Test` instead?
Thanks for spotting this. There is some inconsistency within the project. The top of the github repository is named "Googletest" with directories being "googletest" instead of "google_test", but documentation does use "Google Test".

For now I have changed the message to use "Google Test", but have kept "googletest" in the check name. Let me know if you would prefer "GoogleTest" and "google-test" elsewhere as well.


================
Comment at: clang-tools-extra/clang-tidy/google/UpgradeGoogletestCaseCheck.cpp:201
+                         const MatchFinder::MatchResult &Result) {
+  internal::Matcher<NodeType> IsInsideTemplate =
+      hasAncestor(decl(anyOf(classTemplateDecl(), functionTemplateDecl())));
----------------
EricWF wrote:
> Instead of walking the tree, can't you ask if the Node is dependent in some way?
I think it is a trade-off. Given where this gets called, what we are really looking to gather are all places that are inside a template that are not instantiation dependent. If we only use a check against instantiation dependent, then the hash set gets a lot bigger because it holds every matched location and is not filtered down to the ones inside templates. Let me know if you think I should explore that route instead.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62977/new/

https://reviews.llvm.org/D62977