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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman 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);
----------------
stephanemoore wrote:
> 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.
I don't think we should have the regex as part of the diagnostic either -- it's a distraction and it's not something we typically do. While it's nice for the diagnostic to explain how to fix the issue, the critical bit is diagnosing what's wrong with the code. The diagnostic without the regex does that and users have a place to find that information (the style guide docs).


================
Comment at: test/clang-tidy/google-objc-global-variable-declaration.m:33
 static NSString* const kGood = @"hello";
+static NSString* const XYGood = @"hello";
 static NSString* gMyIntGood = 0;
----------------
stephanemoore wrote:
> aaron.ballman wrote:
> > Can you also add the code from the style guide as a test case?
> > ```
> > extern NSString *const GTLServiceErrorDomain;
> > 
> > typedef NS_ENUM(NSInteger, GTLServiceError) {
> >   GTLServiceErrorQueryResultMissing = -3000,
> >   GTLServiceErrorWaitTimedOut       = -3001,
> > };
> > ```
> NS_ENUM and NSInteger are not defined in this implementation file.
> 
> I can either (1) Omit the macro and the type alias; or (2) reasonably replicate the macro and type alias in this source file.
> 
> Which option is preferred? For the time being, I have pursued option (1) on the assumption that NS_ENUM and NSInteger are not critical to this test case.
I think (1) is totally fine. It's the identifiers we're worried about, not the macros or types.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43581