[PATCH] D43581: [clang-tidy/google] Improve the Objective-C global variable declaration check 🔧

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore added inline comments.


================
Comment at: clang-tidy/google/GlobalVariableDeclarationCheck.cpp:92
+         "an appropriate prefix (see "
+         "http://google.github.io/styleguide/objcguide#constants).")
         << Decl->getName() << generateFixItHint(Decl, true);
----------------
Wizard wrote:
> aaron.ballman wrote:
> > We don't usually put hyperlinks in the diagnostic messages, so please remove this.
> > 
> > My suggestion about describing what constitutes an appropriate prefix was with regards to the style guide wording itself. For instance, that document doesn't mention that two capital letters is good. That's not on you to fix before this patch goes in, of course.
> Btw it is actually "2 or more" characters for prefix. I think it makes sense because we need at least 2 or more characters to call it a "prefix" :-)
Reverted the inclusion of the hyperlink in the message. I agree that embedding the hyperlink in the message is indirect and inconsistent with other diagnostic messages in LLVM projecta.


================
Comment at: clang-tidy/google/GlobalVariableDeclarationCheck.cpp:92
+         "an appropriate prefix (see "
+         "http://google.github.io/styleguide/objcguide#constants).")
         << Decl->getName() << generateFixItHint(Decl, true);
----------------
stephanemoore wrote:
> Wizard wrote:
> > aaron.ballman wrote:
> > > We don't usually put hyperlinks in the diagnostic messages, so please remove this.
> > > 
> > > My suggestion about describing what constitutes an appropriate prefix was with regards to the style guide wording itself. For instance, that document doesn't mention that two capital letters is good. That's not on you to fix before this patch goes in, of course.
> > Btw it is actually "2 or more" characters for prefix. I think it makes sense because we need at least 2 or more characters to call it a "prefix" :-)
> Reverted the inclusion of the hyperlink in the message. I agree that embedding the hyperlink in the message is indirect and inconsistent with other diagnostic messages in LLVM projecta.
§1 Regarding Prefixes And Character Count

Strictly speaking, a single character prefix (e.g., `extern const int ALimit;` ❌) is possible but not allowed for constants in Google Objective-C style except for lowercase 'k' as a prefix for file scope constants (e.g., `static const int kLimit = 3;` ✔️). As hinted in the commit description, Google currently enforces a minimum two character prefix (e.g., `extern const int ABPrefix;` ✔️) or exceptionally 'k' where appropriate.

Technically this modified regex allows authored code to include inadvisable constant names with a single character prefix (e.g., `extern const int APrefix;` ❌). In this respect I would say that this change eliminates an important category of false positives (constants prefixed with '[A-Z]{2,}') while introducing a less important category of false negatives (constants prefixed with only '[A-Z]'). The false positives are observed in standard recommended code while the false negatives occur in non-standard unrecommended code. Without a reliable mechanism to separate the constant prefix from the constant name (e.g., splitting "FOOURL" into "FOO" and "URL" or "HTTP2Header" into "HTTP2" and "Header"), I cannot immediately think of a way to eliminate the introduced category of false negatives without introducing other false positives. I intend to continue thinking about alternative methods that might eliminate these false negatives without introducing other compromises. If it is acceptable to the reviewers I would propose to move forward with this proposed change to eliminate the observed false positives and then consider addressing the false negatives in a followup.

§2 Regarding the Google Objective-C Style Guide's Description of Appropriate Prefixes

I agree that the Google Objective-C style guide should be clearer on this topic and will pursue clarifying this promptly.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43581