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.

  rCTE Clang Tools Extra


cfe-commits mailing list

Reply via email to