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

[Daniel Sanders via Phabricator via cfe-commits]

dsanders marked 3 inline comments as done.
dsanders added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:48-51
+    for (const auto *UsingDirective: Context->using_directives())
+      if (UsingDirective->getNominatedNamespace()
+              ->getQualifiedNameAsString() == "llvm")
+        Replacement = "Register";
----------------
dsanders wrote:
> There's something not quite right here but I haven't figured out what yet. One of the tests (apply_2()) is failing (which is odd because I ran `ninja check-clang-tools` before posting the patch) because the `using namespace llvm` in the function scope doesn't show up in the DeclContext for the function. Am I looking in the wrong place for it?
> 
> More generally, I suspect there's an easier way to omit the `llvm::` than the way I'm using on lines 43-52.
I've given in on trying to elide the `llvm::` for this case as the `using namespace llvm` inside a function doesn't seem to be easily discoverable and doesn't occur in practice


================
Comment at: clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp:53
+  }
+  diag(UserVarDecl->getLocation(), "use '%0'", DiagnosticIDs::Note)
+      << Replacement
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65919