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

[Yan Zhang via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL326046: [clang-tidy/google] Improve the Objective-C global 
variable declaration check 🔧 (authored by Wizard, committed by ).
Herald added subscribers: llvm-commits, klimek.

Changed prior to commit:
  https://reviews.llvm.org/D43581?vs=135745&id=135811#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D43581

Files:
  clang-tools-extra/trunk/clang-tidy/google/GlobalVariableDeclarationCheck.cpp
  
clang-tools-extra/trunk/test/clang-tidy/google-objc-global-variable-declaration.m


Index: 
clang-tools-extra/trunk/clang-tidy/google/GlobalVariableDeclarationCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/google/GlobalVariableDeclarationCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/google/GlobalVariableDeclarationCheck.cpp
@@ -72,7 +72,7 @@
   this);
   Finder->addMatcher(varDecl(hasGlobalStorage(), hasType(isConstQualified()),
  unless(isLocalVariable()),
- unless(matchesName("::k[A-Z]")))
+ unless(matchesName("::(k[A-Z]|[A-Z]{2,})")))
  .bind("global_const"),
  this);
 }
@@ -88,7 +88,7 @@
   if (const auto *Decl = Result.Nodes.getNodeAs("global_const")) {
 diag(Decl->getLocation(),
  "const global variable '%0' must have a name which starts with "
- "'k[A-Z]'")
+ "an appropriate prefix")
 << Decl->getName() << generateFixItHint(Decl, true);
   }
 }
Index: 
clang-tools-extra/trunk/test/clang-tidy/google-objc-global-variable-declaration.m
===
--- 
clang-tools-extra/trunk/test/clang-tidy/google-objc-global-variable-declaration.m
+++ 
clang-tools-extra/trunk/test/clang-tidy/google-objc-global-variable-declaration.m
@@ -2,7 +2,7 @@
 
 @class NSString;
 static NSString* const myConstString = @"hello";
-// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 
'myConstString' must have a name which starts with 'k[A-Z]' 
[google-objc-global-variable-declaration]
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 
'myConstString' must have a name which starts with an appropriate prefix 
[google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* const kMyConstString = @"hello";
 
 static NSString* MyString = @"hi";
@@ -22,16 +22,24 @@
 // CHECK-FIXES: static NSString* gNoDef;
 
 static NSString* const _notAlpha = @"NotBeginWithAlpha";
-// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable '_notAlpha' 
must have a name which starts with 'k[A-Z]' 
[google-objc-global-variable-declaration]
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable '_notAlpha' 
must have a name which starts with an appropriate prefix 
[google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* const _notAlpha = @"NotBeginWithAlpha";
 
 static NSString* const k_Alpha = @"SecondNotAlpha";
-// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 'k_Alpha' 
must have a name which starts with 'k[A-Z]' 
[google-objc-global-variable-declaration]
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 'k_Alpha' 
must have a name which starts with an appropriate prefix 
[google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* const k_Alpha = @"SecondNotAlpha";
 
 static NSString* const kGood = @"hello";
+static NSString* const XYGood = @"hello";
 static NSString* gMyIntGood = 0;
 
+extern NSString* const GTLServiceErrorDomain;
+
+enum GTLServiceError {
+  GTLServiceErrorQueryResultMissing = -3000,
+  GTLServiceErrorWaitTimedOut   = -3001,
+};
+
 @implementation Foo
 - (void)f {
 int x = 0;


Index: clang-tools-extra/trunk/clang-tidy/google/GlobalVariableDeclarationCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/google/GlobalVariableDeclarationCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/google/GlobalVariableDeclarationCheck.cpp
@@ -72,7 +72,7 @@
   this);
   Finder->addMatcher(varDecl(hasGlobalStorage(), hasType(isConstQualified()),
  unless(isLocalVariable()),
- unless(matchesName("::k[A-Z]")))
+ unless(matchesName("::(k[A-Z]|[A-Z]{2,})")))
  .bind("global_const"),
  this);
 }
@@ -88,7 +88,7 @@
   if (const auto *Decl = Result.Nodes.getNodeAs("global_const")) {
 diag(Decl->getLocation(),
  "const global variable '%0' must have a name which starts with "
- "'k[A-Z]'")
+ "an appropriate prefix")
 << Decl->getName() << generateFixItHint(Decl, true);
   }
 }
Index: clang-tools-extra/trunk/test/clang-tidy/google-objc-global-variable-declaration.m

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

[Yan Zhang via Phabricator via cfe-commits]

Wizard added a comment.

In https://reviews.llvm.org/D43581#1018584, @stephanemoore wrote:

> In https://reviews.llvm.org/D43581#1018499, @aaron.ballman wrote:
>
> > This LGTM! Do you need someone to commit on your behalf?
>
>
> I would be happy to commit assuming that I am able to and can meet submission 
> requirements.
>
> I see a "Submit" button in Phabricator that I assume will land the commit if 
> I press it?
>
> I found some submission guidelines in the LLVM Developer Policy 
> . Are there any other submission 
> guidelines I should follow?
>
> I ran the LLVM and Clang regression tests (by running `make check-all` from 
> my LLVM build directory) and I encountered a failure in "Bindings/Go/go.test":
>
>    TEST 'LLVM :: Bindings/Go/go.test' FAILED 
> 
>   Script:
>   --
>   /Users/mog/projects/llvm-build/bin/llvm-go go=/usr/local/go/bin/go test 
> llvm.org/llvm/bindings/go/llvm
>   --
>   Exit Code: 1
>  
>   Command Output (stdout):
>   --
>   FAILllvm.org/llvm/bindings/go/llvm [build failed]
>  
>   --
>   Command Output (stderr):
>   --
>   # llvm.org/llvm/bindings/go/llvm
>   In file included from 
> /var/folders/qh/4y215hgd4zqg30v9q04czw58005k3k/T/lit_tmp_7sZYWR/gopath735545420/src/llvm.org/llvm/bindings/go/llvm/analysis.go:17:
>   In file included from /Users/mog/projects/llvm/include/llvm-c/Analysis.h:22:
>   In file included from /Users/mog/projects/llvm/include/llvm-c/Types.h:17:
>   /Users/mog/projects/llvm-build/include/llvm/Support/DataTypes.h:35:10: 
> fatal error: 'math.h' file not found
>   #include 
>^~~~
>   1 error generated.
>  
>   --
>  
>   
>
>
> I reran these tests on a clean checkout and I still encountered the failure 
> so I presume that this failure is unrelated?
>
> I have also been trying to run the `test-suite` but I have been encountering 
> Python exceptions while trying to run `lnt` 😕.
>
> If someone wants to submit on my behalf that works for me. If not, I can also 
> continue trying to drive this myself (though verification of submission 
> requirements would be helpful in that case).


You may wanna follow this 
http://llvm.org/docs/Phabricator.html#git-svn-and-arcanist to submit your 
changes. But as aaron.ballman@ mentioned, you may need someone who has acl to 
commit it for you. 
For the error you see it is likely because you only updated the tools/extra 
repo. Usually when we sync this repo, we will have to pull parent repos as well 
(i.e. clang repo and llvm repo).


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

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

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore marked an inline comment as done.
stephanemoore added a comment.

In https://reviews.llvm.org/D43581#1018499, @aaron.ballman wrote:

> This LGTM! Do you need someone to commit on your behalf?


I would be happy to commit assuming that I am able to and can meet submission 
requirements.

I see a "Submit" button in Phabricator that I assume will land the commit if I 
press it?

I found some submission guidelines in the LLVM Developer Policy 
. Are there any other submission 
guidelines I should follow?

I ran the LLVM and Clang regression tests (by running `make check-all` from my 
LLVM build directory) and I encountered a failure in "Bindings/Go/go.test":

   TEST 'LLVM :: Bindings/Go/go.test' FAILED 

  Script:
  --
  /Users/mog/projects/llvm-build/bin/llvm-go go=/usr/local/go/bin/go test 
llvm.org/llvm/bindings/go/llvm
  --
  Exit Code: 1
  
  Command Output (stdout):
  --
  FAIL  llvm.org/llvm/bindings/go/llvm [build failed]
  
  --
  Command Output (stderr):
  --
  # llvm.org/llvm/bindings/go/llvm
  In file included from 
/var/folders/qh/4y215hgd4zqg30v9q04czw58005k3k/T/lit_tmp_7sZYWR/gopath735545420/src/llvm.org/llvm/bindings/go/llvm/analysis.go:17:
  In file included from /Users/mog/projects/llvm/include/llvm-c/Analysis.h:22:
  In file included from /Users/mog/projects/llvm/include/llvm-c/Types.h:17:
  /Users/mog/projects/llvm-build/include/llvm/Support/DataTypes.h:35:10: fatal 
error: 'math.h' file not found
  #include 
   ^~~~
  1 error generated.
  
  --
  
  

I reran these tests on a clean checkout and I still encountered the failure so 
I presume that this failure is unrelated?

I have also been trying to run the `test-suite` but I have been encountering 
Python exceptions while trying to run `lnt` 😕.

If someone wants to submit on my behalf that works for me. If not, I can also 
continue trying to drive this myself (though verification of submission 
requirements would be helpful in that case).


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

This LGTM! Do you need someone to commit on your behalf?


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

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

[Stephane Moore via Phabricator via cfe-commits]

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

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

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore updated this revision to Diff 135745.
stephanemoore marked 3 inline comments as done.
stephanemoore added a comment.

Removed the regex from the diagnostic ✂️


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43581

Files:
  clang-tidy/google/GlobalVariableDeclarationCheck.cpp
  test/clang-tidy/google-objc-global-variable-declaration.m


Index: test/clang-tidy/google-objc-global-variable-declaration.m
===
--- test/clang-tidy/google-objc-global-variable-declaration.m
+++ test/clang-tidy/google-objc-global-variable-declaration.m
@@ -2,7 +2,7 @@
 
 @class NSString;
 static NSString* const myConstString = @"hello";
-// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 
'myConstString' must have a name which starts with 'k[A-Z]' 
[google-objc-global-variable-declaration]
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 
'myConstString' must have a name which starts with an appropriate prefix 
[google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* const kMyConstString = @"hello";
 
 static NSString* MyString = @"hi";
@@ -22,16 +22,24 @@
 // CHECK-FIXES: static NSString* gNoDef;
 
 static NSString* const _notAlpha = @"NotBeginWithAlpha";
-// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable '_notAlpha' 
must have a name which starts with 'k[A-Z]' 
[google-objc-global-variable-declaration]
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable '_notAlpha' 
must have a name which starts with an appropriate prefix 
[google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* const _notAlpha = @"NotBeginWithAlpha";
 
 static NSString* const k_Alpha = @"SecondNotAlpha";
-// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 'k_Alpha' 
must have a name which starts with 'k[A-Z]' 
[google-objc-global-variable-declaration]
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 'k_Alpha' 
must have a name which starts with an appropriate prefix 
[google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* const k_Alpha = @"SecondNotAlpha";
 
 static NSString* const kGood = @"hello";
+static NSString* const XYGood = @"hello";
 static NSString* gMyIntGood = 0;
 
+extern NSString* const GTLServiceErrorDomain;
+
+enum GTLServiceError {
+  GTLServiceErrorQueryResultMissing = -3000,
+  GTLServiceErrorWaitTimedOut   = -3001,
+};
+
 @implementation Foo
 - (void)f {
 int x = 0;
Index: clang-tidy/google/GlobalVariableDeclarationCheck.cpp
===
--- clang-tidy/google/GlobalVariableDeclarationCheck.cpp
+++ clang-tidy/google/GlobalVariableDeclarationCheck.cpp
@@ -72,7 +72,7 @@
   this);
   Finder->addMatcher(varDecl(hasGlobalStorage(), hasType(isConstQualified()),
  unless(isLocalVariable()),
- unless(matchesName("::k[A-Z]")))
+ unless(matchesName("::(k[A-Z]|[A-Z]{2,})")))
  .bind("global_const"),
  this);
 }
@@ -88,7 +88,7 @@
   if (const auto *Decl = Result.Nodes.getNodeAs("global_const")) {
 diag(Decl->getLocation(),
  "const global variable '%0' must have a name which starts with "
- "'k[A-Z]'")
+ "an appropriate prefix")
 << Decl->getName() << generateFixItHint(Decl, true);
   }
 }


Index: test/clang-tidy/google-objc-global-variable-declaration.m
===
--- test/clang-tidy/google-objc-global-variable-declaration.m
+++ test/clang-tidy/google-objc-global-variable-declaration.m
@@ -2,7 +2,7 @@
 
 @class NSString;
 static NSString* const myConstString = @"hello";
-// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 'myConstString' must have a name which starts with 'k[A-Z]' [google-objc-global-variable-declaration]
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 'myConstString' must have a name which starts with an appropriate prefix [google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* const kMyConstString = @"hello";
 
 static NSString* MyString = @"hi";
@@ -22,16 +22,24 @@
 // CHECK-FIXES: static NSString* gNoDef;
 
 static NSString* const _notAlpha = @"NotBeginWithAlpha";
-// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable '_notAlpha' must have a name which starts with 'k[A-Z]' [google-objc-global-variable-declaration]
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable '_notAlpha' must have a name which starts with an appropriate prefix [google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* const _notAlpha = @"NotBeginWithAlpha";
 
 static NSString* const k_Alpha = @"SecondNotAlpha";
-// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 'k_Alpha' must have a name which starts with 'k[A-Z

[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



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

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

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore marked an inline comment as not done.
stephanemoore added inline comments.



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;

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.


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

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

[Stephane Moore via Phabricator via cfe-commits]

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

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

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore updated this revision to Diff 135595.
stephanemoore marked 2 inline comments as done.
stephanemoore retitled this revision from "[clang-tidy/google] Fix the 
Objective-C global variable declaration check 🔧" to "[clang-tidy/google] 
Improve the Objective-C global variable declaration check 🔧".
stephanemoore edited the summary of this revision.
stephanemoore added a comment.

I had to make two more fixes to get the tests working:
• Updated the CHECK-MESSAGES in 
`test/clang-tidy/google-objc-global-variable-declaration.m` to the revised 
diagnostic message.
• Removed NS_ENUM and NSInteger from the additions to 
`test/clang-tidy/google-objc-global-variable-declaration.m` because their 
declarations were missing and I assumed that replication of the Foundation 
macro and type alias was not meaningfully important to this test case.

I verified the changes with the following commands:

  $ ${LLVM_ROOT}/tools/clang/tools/extra/test/clang-tidy/check_clang_tidy.py 
${LLVM_ROOT}/tools/clang/tools/extra/test/clang-tidy/google-objc-global-variable-declaration.m
 google-objc-global-variable-declaration /tmp/test && make check-all

Is that satisfactory verification of these changes?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D43581

Files:
  clang-tidy/google/GlobalVariableDeclarationCheck.cpp
  test/clang-tidy/google-objc-global-variable-declaration.m


Index: test/clang-tidy/google-objc-global-variable-declaration.m
===
--- test/clang-tidy/google-objc-global-variable-declaration.m
+++ test/clang-tidy/google-objc-global-variable-declaration.m
@@ -2,7 +2,7 @@
 
 @class NSString;
 static NSString* const myConstString = @"hello";
-// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 
'myConstString' must have a name which starts with 'k[A-Z]' 
[google-objc-global-variable-declaration]
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 
'myConstString' must have a name which starts with an appropriate prefix 
matching '(k[A-Z]|[A-Z]{2,})' [google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* const kMyConstString = @"hello";
 
 static NSString* MyString = @"hi";
@@ -22,16 +22,24 @@
 // CHECK-FIXES: static NSString* gNoDef;
 
 static NSString* const _notAlpha = @"NotBeginWithAlpha";
-// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable '_notAlpha' 
must have a name which starts with 'k[A-Z]' 
[google-objc-global-variable-declaration]
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable '_notAlpha' 
must have a name which starts with an appropriate prefix matching 
'(k[A-Z]|[A-Z]{2,})' [google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* const _notAlpha = @"NotBeginWithAlpha";
 
 static NSString* const k_Alpha = @"SecondNotAlpha";
-// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 'k_Alpha' 
must have a name which starts with 'k[A-Z]' 
[google-objc-global-variable-declaration]
+// CHECK-MESSAGES: :[[@LINE-1]]:24: warning: const global variable 'k_Alpha' 
must have a name which starts with an appropriate prefix matching 
'(k[A-Z]|[A-Z]{2,})' [google-objc-global-variable-declaration]
 // CHECK-FIXES: static NSString* const k_Alpha = @"SecondNotAlpha";
 
 static NSString* const kGood = @"hello";
+static NSString* const XYGood = @"hello";
 static NSString* gMyIntGood = 0;
 
+extern NSString* const GTLServiceErrorDomain;
+
+enum GTLServiceError {
+  GTLServiceErrorQueryResultMissing = -3000,
+  GTLServiceErrorWaitTimedOut   = -3001,
+};
+
 @implementation Foo
 - (void)f {
 int x = 0;
Index: clang-tidy/google/GlobalVariableDeclarationCheck.cpp
===
--- clang-tidy/google/GlobalVariableDeclarationCheck.cpp
+++ clang-tidy/google/GlobalVariableDeclarationCheck.cpp
@@ -72,7 +72,7 @@
   this);
   Finder->addMatcher(varDecl(hasGlobalStorage(), hasType(isConstQualified()),
  unless(isLocalVariable()),
- unless(matchesName("::k[A-Z]")))
+ unless(matchesName("::(k[A-Z]|[A-Z]{2,})")))
  .bind("global_const"),
  this);
 }
@@ -88,7 +88,7 @@
   if (const auto *Decl = Result.Nodes.getNodeAs("global_const")) {
 diag(Decl->getLocation(),
  "const global variable '%0' must have a name which starts with "
- "'k[A-Z]'")
+ "an appropriate prefix matching '(k[A-Z]|[A-Z]{2,})'")
 << Decl->getName() << generateFixItHint(Decl, true);
   }
 }


Index: test/clang-tidy/google-objc-global-variable-declaration.m
===
--- test/clang-tidy/google-objc-global-variable-declaration.m
+++ test/clang-tidy/google-objc-global-variable-declaration.m
@@ -2,7 +2,7 @@
 
 @class NSString;
 static NSString* const myConstString = @"hello";
-// CHECK-MESSAGES: :[[@L