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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman marked an inline comment as done.
aaron.ballman 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>(
----------------
Please keep this list sorted in alphabetical order.


================
Comment at: clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:53
+  }
+  diag(UserVarDecl->getLocation(), "use '%0'", DiagnosticIDs::Note)
+      << Replacement
----------------
dsanders wrote:
> aaron.ballman wrote:
> > dsanders wrote:
> > > aaron.ballman wrote:
> > > > I don't think you should issue a second diagnostic on the same line. Instead, only issue the previous diagnostic with the fixit attached to it.
> > > I don't mind changing this but I thought I should mention that I'm following the example set by the code generated by add_new_check.py which has the diagnostic separate from the note with the fixit:
> > > ```
> > >   diag(MatchedDecl->getLocation(), "function %%0 is insufficiently awesome")
> > >       << MatchedDecl;
> > >   diag(MatchedDecl->getLocation(), "insert 'awesome'", DiagnosticIDs::Note)
> > >       << FixItHint::CreateInsertion(MatchedDecl->getLocation(), "awesome_");
> > > ```
> > > Is the example doing it the right way?
> > That script is intended to generate boilerplate so that you don't have to and to show a very minimal example of how to print output. So it's both correct and not particularly helpful for real checks at the same time, if that makes sense.
> I guess it makes sense to have one example of a warning and one of a note. It might be worth adding a comment suggesting to those new to clang-tidy that they should try to emit a single 'this is wrong; do this' diagnostic rather than the two separate ones from the example but that's not for this patch at least.
> 
> Thanks
Yeah, I wouldn't be opposed to a patch to the python script to make that more clear. Or dropping the note entirely under the assumption the user can look at the `diag()` signature to see how to make warnings vs errors vs notes.


================
Comment at: clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:21
+  auto RegisterClassMatch = hasType(
+      cxxRecordDecl(hasName("llvm::Register")).bind("registerClassDecl"));
+
----------------
Should be `::llvm::Register` but not hugely important as I doubt we care too terribly much about inner namespaces named `llvm`.


================
Comment at: clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:31
+                      has(memberExpr(hasDeclaration(
+                          cxxConversionDecl().bind("operatorDecl"))))))))))))
+              .bind("var"))),
----------------
You don't seem to be using the `operatorDecl` binding -- you can remove it, if it's not intended to be used.


================
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";
----------------
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.)


================
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``.
+
----------------
initialize -> initialization


================
Comment at: clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp:60
+}
+} // end namespace llvm
----------------
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();
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65919