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

Fri Sep 6 05:21:11 PDT 2019

aaron.ballman added inline comments.

================
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();
----------------
dgatwood wrote:
> 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.  :-/
> 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. :-/

I am not certain if it's a bug or not, but we shouldn't be using essentially deprecated APIs to work around bugs elsewhere. I'd really like to know what's going on here. I am looking through the logic of DeclarationName::print() (which is what getNameAsString() eventually calls into) and it doesn't look like it makes any special accommodations for ObjC method declarations, but it does for ObjC selectors. I'm not an ObjC expert, but I think that's what the name of an ObjC method is, isn't it? If so, I think you would do (assuming the methods can only be named using selectors):
```
MethodDeclaration->getDeclName().getObjCSelector().getAsString()
```

================
Comment at: clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:30
+    ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "WhitelistedPrefixes", WhitelistedPrefixes);
+}
----------------
dgatwood wrote:
> 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.).
Ah, thank you for the explanation!

================
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 =
----------------
dgatwood wrote:
> 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.
No worries, things are looking good now!

================
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;
+
----------------
dgatwood wrote:
> 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.
No worries!

================
Comment at: clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst:28
+
+If you whitelist the ``QED`` three-letter prefix, the following code sample
+is also allowed:
----------------
whitelist the QED three-letter prefix -> expect the three-letter prefix QED

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

https://reviews.llvm.org/D65917