aaron.ballman added inline comments.
================ 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? Still wondering whether we should have a default list of expected prefixes or not. ================ Comment at: clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:15 + +static const char *kCustomCategoryMethodIdentifier = "ThisIsACategoryMethod"; + ---------------- Drop the `k` prefix (we don't use Hungarian notation). ================ Comment at: clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:36-40 + auto ClassNameIter = llvm::find_if(PrefixArray, + [ClassName](const std::string &Str) { + return ClassName.startswith(Str); + }); + return ClassNameIter != PrefixArray.end(); ---------------- Sorry for not recognizing this earlier, but since we don't care about which item was found, we can go with: ``` return llvm::any_of(PrefixArray, ...); ``` ================ 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. 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. ================ 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: > `Finder` and `Result` per coding style. `finder` -> `Finder` `result` -> `Result` ================ Comment at: clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.h:31 + llvm::Optional<std::string> + matchingWhitelistedPrefix(StringRef class_name); +}; ---------------- aaron.ballman wrote: > `class_name` should be `ClassName`. `class_name` -> `ClassName` 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