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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
