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