[PATCH] D42464: add prefix with '_' support for property name. Corresponding apple dev doc: https://developer.apple.com/library/content/qa/qa1908/_index.html

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:28
+enum NamingStyle {
+  StandardProperty = 1,
+  CategoryProperty = 2,
----------------
Please add documentation describing what these properties are.


================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:54
 
 /// For now we will only fix 'CamelCase' property to
 /// 'camelCase'. For other cases the users need to
----------------
Do we need to update the documentation here? it seems stale now?


================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:84
 
-std::string validPropertyNameRegex(const std::vector<std::string> &Acronyms) {
+std::string validPropertyNameRegex(const std::vector<std::string> &Acronyms,
+                                   bool UsedInMatcher) {
----------------
Also add documentation for the method as well as parameters. It is a bit hard to follow the logic.


================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:93
+                 [](const std::string &s) { return llvm::Regex::escape(s); });
   // Allow any of these names:
   // foo
----------------
Does the comment still make sense? Seems you changed the regex below.


================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:115
+
+bool prefixedPropertyNameMatches(const llvm::StringRef &PropertyName,
+                                 const std::vector<std::string> &Acronyms) {
----------------
no need to pass const reference of `llvm::StringRef`. `StringRef` is cheap.


================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:116
+bool prefixedPropertyNameMatches(const llvm::StringRef &PropertyName,
+                                 const std::vector<std::string> &Acronyms) {
+  size_t Start = 0;
----------------
nit: use `llvm::ArrayRef<std::string>` would save a lot of keystroke.


================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:118
+  size_t Start = 0;
+  while (PropertyName[Start] != '_') {
+    Start++;
----------------
we can use `find_first_not_of` or something similar.


================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:151
+      hasCategoryPropertyPrefix(MatchedDecl->getName())) {
+    const auto *CategoryDecl = (const ObjCCategoryDecl *)(DeclContext);
+    if (!prefixedPropertyNameMatches(MatchedDecl->getName(), SpecialAcronyms) ||
----------------
Consider using `const auto* CategoryDecl = dyn_cast<ObjCCategoryDecl*>(DeclContext)`, we can get rid of this cast, and `NodeKind` variable.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42464