[PATCH] D62045: Modified global variable declaration to fit updated objc guide.

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore requested changes to this revision.
stephanemoore added a comment.
This revision now requires changes to proceed.

Almost there. I think everything looks good after we resolve this last round of comments.

Can you also update the commit description. I believe that the current changes can be described roughly as follows:
"""
Do not emit fixes for extern global constants in google-objc-global-variable-declaration check.
"""



================
Comment at: clang-tools-extra/test/clang-tidy/google-objc-global-variable-declaration.m:26
 // 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";
 
----------------
stephanemoore wrote:
> Thanks for removing the fixit on this case 😄 The fix was not compliant with Google Objective-C style.
Oh wait. I believe this was here to verify that no fix was recommended. We should restore this.

(sorry for the error on my part 🤦)


================
Comment at: clang-tools-extra/test/clang-tidy/google-objc-global-variable-declaration.m:30
-// 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";
 
----------------
stephanemoore wrote:
> Thanks for removing the fixit on this case 😄 The fix was not compliant with Google Objective-C style.
Oh wait. I believe this was here to verify that no fix was recommended. We should restore this.

(sorry for the error on my part 🤦)


================
Comment at: clang-tools-extra/test/clang-tidy/google-objc-global-variable-declaration.m:5
+
+static NSString *const myConstString = @"hello";
 // 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]
----------------
In general I recommend splitting stylistic changes independently of functional changes so that it's easy to identify changes that actually affected behavior.

I recommend one of the two following actions:
(1) Expand the commit description to describe the reformatting changes to `google-objc-global-variable-declaration.m` so that it's clear to readers that these were intentional changes included in this commit.
(2) Split out the reformatting changes unrelated to resolving the inconsistency between the check and the Objective-C style guide into an independent commit and submit that for independent review.


================
Comment at: clang-tools-extra/test/clang-tidy/google-objc-global-variable-declaration.m:45
 
-extern NSString* const GTLServiceErrorDomain;
+extern NSString *const GTLServiceErrorDomain;
 
----------------
Can we add a test case as follows:
```
extern NSString *Y2Good;
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62045/new/

https://reviews.llvm.org/D62045