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