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