aaron.ballman added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/google/GoogleTidyModule.cpp:44
+ CheckFactories.registerCheck<objc::RequireCategoryMethodPrefixesCheck>(
+ "google-objc-require-category-method-prefixes");
CheckFactories.registerCheck<build::ExplicitMakePairCheck>(
----------------
Please keep this list in alphabetical order.
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:21
+ // This check should be run only on Objective-C code.
+ if (!getLangOpts().ObjC) {
+ return;
----------------
You should elide the braces here per our usual coding conventions (also applies
elsewhere).
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:25
+
+ auto method_declaration =
+ objcMethodDecl().bind(kCustomCategoryMethodIdentifier);
----------------
No need for the local here, I'd just inline it below.
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:37
+RequireCategoryMethodPrefixesCheck::matching_whitelisted_prefix(
+ std::string class_name) {
+ std::vector<std::string> prefix_array =
----------------
I think this interface should accept a `StringRef` argument and either return a
`std::string` or an `llvm::Optional<std::string>` rather than allocating a
pointer.
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:43
+ const char *prefix = iterator.c_str();
+ if (!strncmp(class_name.c_str(), prefix, strlen(prefix))) {
+ return llvm::make_unique<std::string>(prefix);
----------------
No need for messy C APIs here -- you can compare a `StringRef` to a
`std::string` directly.
================
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();
----------------
This should use `getName()` to get a `StringRef` to avoid the copy.
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:62
+ }
+ std::string class_name = owning_objc_class_interface->getNameAsString();
+
----------------
Can use `getName()` and assign to a `StringRef`
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:100
+ diag(method_declaration->getBeginLoc(),
+ "the category method %0 is not properly prefixed.")
+ << method_name;
----------------
Drop the period at the end of the diagnostic -- clang-tidy diagnostics are not
meant to be grammatically correct.
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.cpp:101
+ "the category method %0 is not properly prefixed.")
+ << method_name;
+}
----------------
You should pass `method_declaration` instead; it will get quoted and converted
to the proper identifier automatically.
================
Comment at:
clang-tools-extra/clang-tidy/google/RequireCategoryMethodPrefixesCheck.h:28
+ /// Defaults to empty.
+ const std::string whitelisted_prefixes_;
+
----------------
You should pick names that match the LLVM coding style naming rules (here and
elsewhere in the patch).
Repository:
rG LLVM Github Monorepo
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
