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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
