aaron.ballman added inline comments.
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:16
+namespace {
+const char *kCustomCategoryMethodIdentifier = "ThisIsACategoryMethod";
+} // anonymous namespace
----------------
Eugene.Zelenko wrote:
> Please use static. See LLVM Coding Guidelines.
This comment also meant that you should drop the anonymous namespace (per our
coding conventions -- we only use anonymous namespaces for types).
================
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:
> > 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.
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:19
+
+void RequireCategoryMethodPrefixesCheck::registerMatchers(MatchFinder *finder)
{
+ // This check should be run only on Objective-C code.
----------------
`Finder`
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:30
+ ClangTidyOptions::OptionMap &Opts) {
+ Options.store(Opts, "WhitelistedPrefixes", WhitelistedPrefixes);
+}
----------------
`ExpectedPrefixes` here as well.
Should there be a default list of these?
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:39-43
+ for (auto &Prefix : PrefixArray) {
+ if (class_name.startswith(Prefix)) {
+ return Prefix;
+ }
+ }
----------------
`llvm::find_if(PrefixArray, [ClassName](const std::string &Str) { return
ClassName.startswith(Str); });`
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:49
+ const MatchFinder::MatchResult &result) {
+ const clang::ObjCMethodDecl *method_declaration =
+ result.Nodes.getNodeAs<ObjCMethodDecl>(kCustomCategoryMethodIdentifier);
----------------
`MethodDeclaration`
================
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 =
----------------
`MethodName` (and so on, I'll stop commenting on them.)
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:57
+ StringRef method_name = method_declaration->getNameAsString();
+ const clang::ObjCInterfaceDecl *owning_objc_class_interface =
+ method_declaration->getClassInterface();
----------------
Drop the `clang::`
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:64
+
+ const clang::ObjCCategoryDecl *OwningObjCCategory =
+ dyn_cast<clang::ObjCCategoryDecl>(method_declaration->getDeclContext());
----------------
Can use `const auto *` because the type is in the initialization. However, drop
the `clang::` as it's not needed.
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:75
+
+ if (MatchingPrefix.hasValue())
+ return;
----------------
I notice we never actually care about the string contained within
`MatchingPrefix`, which suggests that `matchingWhitelistedPrefix()` could just
return a `bool`.
================
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;
+
----------------
`Finder` and `Result` per coding style.
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.h:28-31
+ const std::string WhitelistedPrefixes;
+
+ llvm::Optional<std::string>
+ matchingWhitelistedPrefix(StringRef class_name);
----------------
I think the name `ExpectedPrefixes` and `matchesExpectedPrefix()` are more
clear.
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.h:31
+ llvm::Optional<std::string>
+ matchingWhitelistedPrefix(StringRef class_name);
+};
----------------
`class_name` should be `ClassName`.
================
Comment at:
clang-tools-extra/docs/clang-tidy/checks/google-objc-require-category-method-prefixes.rst:4
+google-objc-require-category-method-prefixes
+===========================
+
----------------
Underlining looks off here.
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
