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

[Stephane Moore via Phabricator via cfe-commits]

stephanemoore added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/google/GlobalVariableDeclarationCheck.cpp:45
+
+  auto NewName = (IsConst? "k": "g") + llvm::StringRef(std::string(1, FC)).upper()) +
+                           Decl->getName().substr(1).str());
----------------
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.


================
Comment at: clang-tools-extra/clang-tidy/google/GlobalVariableDeclarationCheck.cpp:45
+
+  auto NewName = (IsConst? "k": "g") + llvm::StringRef(std::string(1, FC)).upper()) +
+                           Decl->getName().substr(1).str());
----------------
stephanemoore wrote:
> 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.
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.


================
Comment at: clang-tools-extra/clang-tidy/google/GlobalVariableDeclarationCheck.cpp:74
+              unless(isLocalVariable()),
+              unless(matchesName("::(k[A-Z].*)|([A-Z][A-Z0-9].*)")))
+          .bind("global_const"),
----------------
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)


================
Comment at: clang-tools-extra/test/clang-tidy/google-objc-global-variable-declaration.m:28
 
-static NSString* const k_Alpha = @"SecondNotAlpha";
-// 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]
----------------
I would keep a `k_Alpha` test case so that we know that an underscore after the `k` is not accepted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62045