[PATCH] D65917: [clang-tidy] Added check for the Google style guide's category method naming rule.

[Aaron Ballman via Phabricator via cfe-commits]

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