hokein added inline comments.
================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:115
+
+bool prefixedPropertyNameMatches(const llvm::StringRef &PropertyName,
+ const std::vector<std::string> &Acronyms) {
----------------
benhamilton wrote:
> 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`.
Yeah, this is what I mean, thanks for the clarifying.
================
Comment at: clang-tidy/objc/PropertyDeclarationCheck.cpp:151
+ hasCategoryPropertyPrefix(MatchedDecl->getName())) {
+ const auto *CategoryDecl = (const ObjCCategoryDecl *)(DeclContext);
+ if (!prefixedPropertyNameMatches(MatchedDecl->getName(), SpecialAcronyms)
||
----------------
Wizard wrote:
> benhamilton wrote:
> > 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.)
> >
> Finally figured out that I cannot put pointer type in dyn_cast<>. It needs to
> be "llvm::dyn_cast<ObjCCategoryDecl>(DeclContext)" even though DeclContext is
> a pointer. I have to say it is weird in C++...
Yeah, llvm::dyn_cast doesn't need to specify the pointer, sorry for my
beginning comment...
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42464
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
