Page MenuHomePhabricator

Revise the google-objc-global-variable-declaration check to match the style guide.
ClosedPublic

Authored by yaqiji on May 16 2019, 4:23 PM.

Details

Summary

Revise the google-objc-global-variable-declaration check to match the style guide.

This commit updates the check as follows:
(1) Do not emit fixes for extern global constants.
(2) Allow the second character of prefixes for constants to be numeric (the new guideline is that global constants should generally be named with a prefix that begins with a capital letter followed by one or more capital letters or numbers).

https://google.github.io/styleguide/objcguide.html#prefixes

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Thanks for restoring the support for the legacy style! I noticed that there are some cases where we can preserve an existing fixit.

clang-tools-extra/clang-tidy/google/GlobalVariableDeclarationCheck.cpp
45 ↗(On Diff #200102)

We can still emit a fix for static constants. Perhaps only early return if the variable declaration is both constant and static?

clang-tools-extra/test/clang-tidy/google-objc-global-variable-declaration.m
6 ↗(On Diff #200102)

This is still a valid fix that we can emit.

Maybe we can just recommend a fix for static constants?

Maybe we can add a case that we don't believe should have an emitted fixit like so:

extern NSString *const GlobalConstant;
This revision now requires changes to proceed.May 17 2019, 2:58 PM
yaqiji updated this revision to Diff 200113.May 17 2019, 4:16 PM
yaqiji marked an inline comment as done.

Added fixithint for const as well.

stephanemoore added inline comments.May 17 2019, 6:06 PM
clang-tools-extra/clang-tidy/google/GlobalVariableDeclarationCheck.cpp
45 ↗(On Diff #200113)

Can you make sure this is formatted correctly? I would expect a space before the ? but I am not familiar with LLVM style on every detail.

45 ↗(On Diff #200113)

Do the tests pass with the added GlobalConstant test case? I would expect that we need to add an early return to handle extern global constants but I could be mistaken.

74 ↗(On Diff #200113)

What is the reason for changing the regex to match the entire name?

(I do not object to the change but I want to know if there is a policy that I am not aware of)

clang-tools-extra/test/clang-tidy/google-objc-global-variable-declaration.m
28 ↗(On Diff #200113)

I would keep a k_Alpha test case so that we know that an underscore after the k is not accepted.

yaqiji updated this revision to Diff 200399.May 20 2019, 9:46 PM
yaqiji marked 2 inline comments as done.

Modified CMakeList to include tests, otherwise clangTidyGoogleModule wouldn't compile.
Added k_Alpha test case back.
Added Extern case so that if it is a extern global constant, no suggestion will show up.
Removed .* and leave substrings only for matching.

yaqiji updated this revision to Diff 200401.May 20 2019, 9:49 PM

Simplified method signature.

stephanemoore requested changes to this revision.May 21 2019, 4:53 PM

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.
"""

clang-tools-extra/test/clang-tidy/google-objc-global-variable-declaration.m
5 ↗(On Diff #200401)

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.

45 ↗(On Diff #200401)

Can we add a test case as follows:

extern NSString *Y2Good;
26 ↗(On Diff #199944)

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 🤦)

30 ↗(On Diff #199944)

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 🤦)

This revision now requires changes to proceed.May 21 2019, 4:53 PM
yaqiji retitled this revision from Modified global variable declaration to fit updated objc guide. to Do not emit fixes for extern global constants in google-objc-global-variable-declaration check..May 21 2019, 5:29 PM
yaqiji edited the summary of this revision. (Show Details)
yaqiji updated this revision to Diff 200613.May 21 2019, 5:36 PM
yaqiji marked 4 inline comments as done.

Restored fix.
Added test case for extern Cap+Number prefixes.
Restored reformatting.

yaqiji updated this revision to Diff 200614.May 21 2019, 5:38 PM

Removed extra space.

stephanemoore requested changes to this revision.May 21 2019, 6:16 PM

Many thanks for being patient with me 🙏 I think there are just two more things ✌️

Sorry, I think my earlier description was understated. Maybe this would be a better commit description:
"""
Revise the google-objc-global-variable-declaration check to match the style guide.

This commit updates the check as follows:
(1) Do not emit fixes for extern global constants.
(2) Allow the second character of prefixes for constants to be numeric (the new guideline is that global constants should generally be named with a prefix that begins with a capital letter followed by one or more capital letters or numbers).

https://google.github.io/styleguide/objcguide.html#prefixes
"""

(sorry I am a bit disorganized since I have been juggling various things today)

clang-tools-extra/clang-tidy/google/CMakeLists.txt
17 ↗(On Diff #200614)

This change doesn't look like it belongs in this commit? Please revert.

This revision now requires changes to proceed.May 21 2019, 6:16 PM
yaqiji retitled this revision from Do not emit fixes for extern global constants in google-objc-global-variable-declaration check. to Revise the google-objc-global-variable-declaration check to match the style guide..May 21 2019, 8:52 PM
yaqiji edited the summary of this revision. (Show Details)
yaqiji updated this revision to Diff 200637.May 21 2019, 8:58 PM
yaqiji edited the summary of this revision. (Show Details)

Removed other mdodified file.

stephanemoore accepted this revision.May 24 2019, 3:13 PM
stephanemoore marked 2 inline comments as done.

(sorry I forgot to send this earlier)

Two small things and then I think everything is good.

clang-tools-extra/clang-tidy/google/GlobalVariableDeclarationCheck.cpp
29 ↗(On Diff #200637)

This condition is probably technically fine since I don't believe it's possible for variables with auto or register storage classes to get here but if we were worried about that then I think we could change the condition to something like this:

if (IsConst && (Decl->getStorageClass() != SC_Static)) {
clang-tools-extra/test/clang-tidy/google-objc-global-variable-declaration.m
16–18 ↗(On Diff #200637)

This is definitely outside the scope of this commit but this fix seems like it would only be appropriate in an implementation file 🤔

32 ↗(On Diff #200637)

Isn't this a case where we would expect kNotCap as a suggested fixit?

39 ↗(On Diff #200637)

Isn't this a case where we would expect kSecondNotCap as a suggested fixit?

yaqiji updated this revision to Diff 201352.May 24 2019, 3:36 PM
yaqiji marked 3 inline comments as done.

Added fix message, and change global to nonstatic.

stephanemoore accepted this revision.May 24 2019, 3:43 PM

Looks good! Thanks for being patient with me!

Thank you so much for reviewing the code and providing great feedbacks!

benhamilton accepted this revision.May 28 2019, 9:36 AM
This revision is now accepted and ready to land.May 28 2019, 9:36 AM

It looks like all concerns have been addressed. Do you need me to land this commit for you?

Yes please, thank you :D

This revision was automatically updated to reflect the committed changes.
stephanemoore reopened this revision.May 28 2019, 7:29 PM
stephanemoore added inline comments.
test/clang-tidy/google-objc-global-variable-declaration.m
48 ↗(On Diff #201806)

Whoops, we forgot the warning that should be generated for this case 😅

Test failure(s):
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/49124
http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/49124/steps/test/logs/stdio

I had to rollback the commit. Can you update the patch to resolve the test failure(s)?

This revision is now accepted and ready to land.May 28 2019, 7:29 PM
stephanemoore requested changes to this revision.May 28 2019, 7:30 PM
This revision now requires changes to proceed.May 28 2019, 7:30 PM
yaqiji updated this revision to Diff 201968.May 29 2019, 9:50 AM

Updated warning

yaqiji updated this revision to Diff 201969.May 29 2019, 9:53 AM

Added check-fix

yaqiji updated this revision to Diff 201971.May 29 2019, 10:18 AM

Removed check-fixes for those without fixithint.
Changed name of Y2Good to Y2Bad to match meaning.

yaqiji updated this revision to Diff 201972.May 29 2019, 10:18 AM

Remove cmake content

yaqiji updated this revision to Diff 201973.May 29 2019, 10:20 AM

Added to check no fixit is generated.

stephanemoore accepted this revision.May 29 2019, 8:43 PM
stephanemoore marked an inline comment as done.
stephanemoore added inline comments.
clang-tools-extra/test/clang-tidy/google-objc-global-variable-declaration.m
46 ↗(On Diff #201973)

This is interesting as it respects the Google Objective-C Style Guide as it is currently written. I think the truth might actually be that there are no guidelines for non-const extern variables as they are incredibly rare and presumably ubiquitously discouraged. In that respect, I am not sure that the fix here represents what would actually be recommended for Google Objective-C. With that said, the fix might be reasonable to emit based on the expectation that it would be exceedingly rare and it's unclear what better guidance we would provide. I think it's fine to continue allowing this existing behavior and continue monitoring.

This revision is now accepted and ready to land.May 29 2019, 8:43 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2019, 4:38 PM

Sorry for jumping in late, but renaming the declaration is not enough -- usages should also be updated; otherwise the developer is better off using a refactoring in their IDE or even a textual search and replace.