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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
