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

[Yan Zhang via Phabricator via cfe-commits]

Wizard added inline comments.


================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:102
+bool hasCategoryPropertyPrefix(const llvm::StringRef &PropertyName) {
+  for (size_t i = 0; i < PropertyName.size() - 1; ++i) {
+    if (PropertyName[i] == '_') {
----------------
benhamilton wrote:
> I think this is an off-by-one error, right? Change:
> 
>   i < PropertyName.size() - 1
> 
> to
> 
>   i < PropertyName.size()
> 
I was thinking of not letting go anything that ends with "_" otherwise I have to do more sanity check later.


================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:103-105
+    if (PropertyName[i] == '_') {
+      return true;
+    }
----------------
benhamilton wrote:
> Do we really want a leading _ to count? I think this might need to be a regular expression instead, something like:
> 
>   ^[a-zA-Z][a-zA-Z0-9]+_[a-zA-Z0-9][a-zA-Z0-9_]*$
> 
Yes it is better to use a regex instead. I would use ^[a-z]+_[a-zA-Z0-9][a-zA-Z0-9_]+$ to make sure we are not having anything like foo_


================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:120
+  auto RegexExp = llvm::Regex(
+      llvm::StringRef(validPropertyNameRegex(Acronyms).replace(0, 2, "^")));
+  return RegexExp.match(llvm::StringRef(PropertyName.substr(Start + 1)));
----------------
benhamilton wrote:
> I don't understand what this is doing. Why are we replacing things in a regex?
> 
> I don't think this is very maintainable. Can you rewrite it for legibility, please?
It is a little awkward here because in the matcher, the regex use "::" to indicate matching start, but in llvm::regex it is the conventional "^". I will add some comments here.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42464