[PATCH] D65917: [clang-tidy] Added check for the Google style guide's category method naming rule.

[David Gatwood via Phabricator via cfe-commits]

dgatwood added a comment.

Ah.  I expected my comments to be submitted as part of uploading a new revision.  Still learning this UI.  :-/



================
Comment at: clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:57
+  }
+  std::string method_name = method_declaration->getNameAsString();
+  auto owning_objc_class_interface = method_declaration->getClassInterface();
----------------
aaron.ballman wrote:
> dgatwood wrote:
> > aaron.ballman wrote:
> > > This should use `getName()` to get a `StringRef` to avoid the copy.
> > That's actually what I originally tried, but that method won't work here, unless I'm missing something.  The getName() method crashes with a message saying that "Name is not a simple identifier".
> You can call `getIdentifier()` instead, and if you get a non-null object back, you can call `getName()` on that. If it is null, there's nothing to check.
I just tried it, and getIdentifier() returns NULL consistently for every category method, so I changed it back to getNameAsString(), which works.


================
Comment at: clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:57
+  }
+  std::string method_name = method_declaration->getNameAsString();
+  auto owning_objc_class_interface = method_declaration->getClassInterface();
----------------
aaron.ballman wrote:
> dgatwood wrote:
> > aaron.ballman wrote:
> > > dgatwood wrote:
> > > > aaron.ballman wrote:
> > > > > This should use `getName()` to get a `StringRef` to avoid the copy.
> > > > That's actually what I originally tried, but that method won't work here, unless I'm missing something.  The getName() method crashes with a message saying that "Name is not a simple identifier".
> > > You can call `getIdentifier()` instead, and if you get a non-null object back, you can call `getName()` on that. If it is null, there's nothing to check.
> > I just tried it, and getIdentifier() returns NULL consistently for every category method, so I changed it back to getNameAsString(), which works.
> The comment to use `getIdentifier()` was marked as done but the changes were not applied; was that a mistake? I'm pushing back on `getNameAsString()` because the function is commented as having its use discouraged, so we should not be adding new uses of it.
I marked that as done because I tried it and it didn't work.  The getIdentifier() method returned NULL for every category method.

BTW, this isn't my first attempt at writing this code in a way that doesn't require that method.  I literally fought with getting the name of category methods for a day or more when I first started writing this, because I kept getting NULLs or crashes.  At one point, I think I even tried looking for the owning class and querying its interface.  Nothing worked until I discovered getNameAsString().

I'm assuming that this is simply a bug somewhere in the LLVM core that nobody has noticed or bothered to fix, because it really should not be difficult to get the name of a method.  :-/


================
Comment at: clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:30
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "WhitelistedPrefixes", WhitelistedPrefixes);
+}
----------------
aaron.ballman wrote:
> `ExpectedPrefixes` here as well.
> 
> Should there be a default list of these?
Done.  And no, there should be no default, unless somehow Xcode's project prefix makes it down as far as LLVM, in which case //maybe// that could be the default.

The idea is that you can whitelist your own Xcode project's prefix, along with the prefixes of your own in-house libraries, so that each individual team/workgroup can add categories on their own classes, but will get warned when they try to add unprefixed category methods on classes that they don't own (e.g. classes in system frameworks, third-party frameworks, etc.).


================
Comment at: clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:30
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "WhitelistedPrefixes", WhitelistedPrefixes);
+}
----------------
aaron.ballman wrote:
> dgatwood wrote:
> > aaron.ballman wrote:
> > > `ExpectedPrefixes` here as well.
> > > 
> > > Should there be a default list of these?
> > Done.  And no, there should be no default, unless somehow Xcode's project prefix makes it down as far as LLVM, in which case //maybe// that could be the default.
> > 
> > The idea is that you can whitelist your own Xcode project's prefix, along with the prefixes of your own in-house libraries, so that each individual team/workgroup can add categories on their own classes, but will get warned when they try to add unprefixed category methods on classes that they don't own (e.g. classes in system frameworks, third-party frameworks, etc.).
> Still wondering whether we should have a default list of expected prefixes or not.
This is weird.  I don't know why this comment system didn't submit my comment before.

No, there should be no default, unless somehow Xcode's project prefix makes it down as far as LLVM, in which case maybe that could be the default.

The idea is that you can whitelist your own Xcode project's prefix, along with the prefixes of your own in-house libraries, so that each individual team/workgroup can add categories on their own classes, but will get warned when they try to add unprefixed category methods on classes that they don't own (e.g. classes in system frameworks, third-party frameworks, etc.).


================
Comment at: clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:56
+  // the error "Name is not a simple identifier".
+  StringRef method_name = method_declaration->getNameAsString();
+  const clang::ObjCInterfaceDecl *owning_objc_class_interface =
----------------
aaron.ballman wrote:
> `MethodName` (and so on, I'll stop commenting on them.)
Yeah.  I finally realized I should just grep for '_' instead of looking for them by hand — one of the joys of converting something originally written under Google's style guide so that it conforms to the llvm style guide instead.  Sorry about that.  I hope I got all of them this time.


================
Comment at: clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:75
+
+  if (MatchingPrefix.hasValue())
+    return;
----------------
aaron.ballman wrote:
> I notice we never actually care about the string contained within `MatchingPrefix`, which suggests that `matchingWhitelistedPrefix()` could just return a `bool`.
We could.  Originally, I was printing it for diagnostics,  but now that I ripped out the debug logging, it isn't really needed.  Done.


================
Comment at: clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.h:22-23
+
+  void registerMatchers(ast_matchers::MatchFinder *finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &result) override;
+
----------------
aaron.ballman wrote:
> aaron.ballman wrote:
> > `Finder` and `Result` per coding style.
> `finder` -> `Finder`
> `result` -> `Result`
What the heck?  Oh, I fixed it in the .cc file and not the .h file.  Sorry.  Done.


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

https://reviews.llvm.org/D65917