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

[Ben Hamilton via Phabricator via cfe-commits]

benhamilton requested changes to this revision.
benhamilton added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:115
+
+bool prefixedPropertyNameMatches(const llvm::StringRef &PropertyName,
+                                 const std::vector<std::string> &Acronyms) {
----------------
Wizard wrote:
> hokein wrote:
> > Wizard wrote:
> > > hokein wrote:
> > > > no need to pass const reference of `llvm::StringRef`. `StringRef` is cheap.
> > > The original property name I directly get from ast is a const. If I remove the const here, I will have to make a copy of the property name before calling this.
> > > I prefer keep this const to save that copy :-)
> > Why? `MatchedDecl->getName()` returns `llvm::StringRef`. As the name indicates, StringRef is a const reference of String, using StringRef is sufficient.
> > 
> > The same to ArrayRef. So the function is like `bool prefixedPropertyNameMatches(llvm::StringRef PropertyName, llvm::ArrayRef<std::string> Acronyms)`.
> > 
> > 
> If I remove the const in the method, compiler will have error of "candidate function not viable: expects an l-value for 1st argument". I believe it cannot accept a const reference as arg if the definition is var.
I think hokein@ is saying you should change:

  const llvm::StringRef &

to:

  llvm::StringRef

Note removing both `const` and `&`, not just removing `const`. This is because there is no need to pass these by reference, they are already a reference.

Same with `const llvm::ArrayRef &`, it should just be `llvm::ArrayRef`.


================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:151
+      hasCategoryPropertyPrefix(MatchedDecl->getName())) {
+    const auto *CategoryDecl = (const ObjCCategoryDecl *)(DeclContext);
+    if (!prefixedPropertyNameMatches(MatchedDecl->getName(), SpecialAcronyms) ||
----------------
Wizard wrote:
> hokein wrote:
> > Wizard wrote:
> > > hokein wrote:
> > > > Consider using `const auto* CategoryDecl = dyn_cast<ObjCCategoryDecl*>(DeclContext)`, we can get rid of this cast, and `NodeKind` variable.
> > > Tried that before but I encountered 2 issues:
> > > 1. 'clang::DeclContext' is not polymorphic
> > > 2. cannot use dynamic_cast with -fno-rtti
> > > which are preventing me from using dynamic_cast
> > Sorry, I mean `llvm::dyn_cast` here, it should work.
> It is not working either. It says "error: static_cast from 'clang::Decl *' to 'const clang::ObjCCategoryDecl *const *' is not allowed" though I have no idea why it is regarded as a static cast.
> I am using it like this:
> const auto *CategoryDecl = llvm::dyn_cast<const ObjCCategoryDecl *>(DeclContext);
You definitely don't want to use a C-style cast. My guess is adding const is not part of what dyn_cast<> does, so you can probably remove that from the dyn_cast<>. (You can of course assign to a const pointer if you want.)



================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:125
+      llvm::Regex(llvm::StringRef(validPropertyNameRegex(Acronyms, false)));
+  return RegexExp.match(llvm::StringRef(PropertyName.substr(Start + 1)));
 }
----------------
No need to construct another llvm::StringRef(), StringRef::substr() returns a StringRef already.



Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42464