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