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