[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 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();
----------------
aaron.ballman wrote:
> 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()
> ```
Yeah, any Objective-C method inside a class is inherently named by a selector.  That code works.  Thanks.  :-)


================
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:
----------------
aaron.ballman wrote:
> whitelist the QED three-letter prefix -> expect the three-letter prefix QED
Applied approximately.  :-)


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

https://reviews.llvm.org/D65917