[PATCH] D65919: [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM

[Daniel Sanders via Phabricator via cfe-commits]

dsanders added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp:28-29
     CheckFactories.registerCheck<IncludeOrderCheck>("llvm-include-order");
+    CheckFactories.registerCheck<PreferRegisterOverUnsignedCheck>(
+        "llvm-prefer-register-over-unsigned");
     CheckFactories.registerCheck<readability::NamespaceCommentCheck>(
----------------
aaron.ballman wrote:
> Please keep this list sorted in alphabetical order.
This change was made by add_new_check.py and looking at the code, it's been confused by the `readability::`. I'll move it to the right place and add a `using readability::NamespaceCommentCheck` so we can drop the `readability::` and have add_new_check.py get it right in future.


================
Comment at: clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:45
+    if (const auto *Namespace = dyn_cast<NamespaceDecl>(Context))
+      if (Namespace->getQualifiedNameAsString() == "llvm")
+        Replacement = "Register";
----------------
aaron.ballman wrote:
> This is a fairly expensive operation compared to calling `getName()`; are there situations where you think you need the extra work to be done? (Same comment below.)
None that are likely to occur. `getName()` with some way to check that there's no enclosing namespace would be equivalent. Is there a cheap way to tell if a namespace is at the top level?


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst:10
+Currently this works by finding all variables of unsigned integer type whose
+initialize begins with an implicit cast from ``Register`` to ``unsigned``.
+
----------------
aaron.ballman wrote:
> initialize -> initialization
It was meant to be 'initializer'. I'll correct that


================
Comment at: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp:60
+}
+} // end namespace llvm
----------------
aaron.ballman wrote:
> I'd also like to see some tests like:
> ```
> void func(unsigned Reg);
> 
> struct S {
>   unsigned Reg;
> 
>   S(unsigned Reg) : Reg(Reg) {}
> };
> 
> void other_func() {
>   func(getReg());
>   S s{getReg()};
>   s.Reg = getReg();
> }
> ```
Added tests for the following cases that do not make changes*
1. Similar interface but not called Register
2. Register class not inside llvm namespace
3. Calling a function that takes an llvm::Register with a llvm::Register argument
4. Calling a function that takes an unsigned and is given an llvm::Register argument
5. Initializing an llvm::Register using an llvm::Register argument
6. Initializing any other object using a constructor parameter that's unsigned and a llvm::Register argument
7. Assigning to a member of llvm::Register
8. Assigning to a unsigned member of any other object

*4, 6, and 8 should eventually but don't yet


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65919