[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/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