stephanemoore marked 2 inline comments as done.
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);
----------------
aaron.ballman wrote:
> 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).
Sounds good to me ๐Ÿ‘Œ Removed the regex from the diagnostic.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43581



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to