This clang-tidy check is looking for unsigned integer variables whose initializer
starts with an implicit cast from llvm::Register and changes the type of the
variable to llvm::Register (dropping the llvm:: where possible).
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
I think patch should be split at least on Clang-tidy check and results of its run on LLVM code. Probably per-target patches is even better solution.
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp | ||
---|---|---|
48 ↗ | (On Diff #214051) | You could use const auto * for iterators. |
clang-tools-extra/docs/ReleaseNotes.rst | ||
85 ↗ | (On Diff #214051) | Please use double back-ticks to highlight language constructs. Same in documentation. |
I've split the LLVM changes out into D65962. I expect it will be split up further for commit purposes if nothing else but I've kept it in a single review for now.
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp | ||
---|---|---|
40 ↗ | (On Diff #214194) | How about variable %0 declared as %1; use '%2' instead and move it below to where the fix-it is issued? |
44 ↗ | (On Diff #214194) | Formatting looks off here, you should run the patch through clang-format. |
53 ↗ | (On Diff #214194) | 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. |
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp | ||
---|---|---|
53 ↗ | (On Diff #214194) | 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? |
Changed diagnostic message and merged the fixit into the original diagnostic
Made the variable names more distinct in the tests
Added a test to cover using namespace llvm in the global namespace
Slight correction to the iteration over the contexts to avoid skipping one
clang-format
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp | ||
---|---|---|
48–51 ↗ | (On Diff #214194) | 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. |
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp | ||
---|---|---|
53 ↗ | (On Diff #214194) | 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. |
- Give in on eliding the llvm:: for using namespace llvm inside a function They don't appear in the DeclContext and I've been unable to find a way to reasonably detect them Fortunately they don't occur in practice
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp | ||
---|---|---|
48–51 ↗ | (On Diff #214194) | 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 |
53 ↗ | (On Diff #214194) | 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 |
clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp | ||
---|---|---|
28–29 ↗ | (On Diff #214476) | Please keep this list sorted in alphabetical order. |
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp | ||
53 ↗ | (On Diff #214194) | 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. |
21 ↗ | (On Diff #214476) | Should be ::llvm::Register but not hugely important as I doubt we care too terribly much about inner namespaces named llvm. |
31 ↗ | (On Diff #214476) | You don't seem to be using the operatorDecl binding -- you can remove it, if it's not intended to be used. |
45 ↗ | (On Diff #214476) | 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.) |
clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst | ||
10 ↗ | (On Diff #214476) | initialize -> initialization |
clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp | ||
60 ↗ | (On Diff #214476) | 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(); } |
- Correct alphabetical list in LLVMTidyModule.cpp and prevent add_new_check.py getting it wrong in future
- Lookup Register class via ::llvm::Register instead of llvm::Register
- Removed unnecessary bindings
- Typo in the docs
- More tests
Haven't made the change to avoid getQualifiedNameAsString() yet in case there's an easy way to tell
if the llvm namespace found is inside another namespace.
clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp | ||
---|---|---|
28–29 ↗ | (On Diff #214476) | 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. |
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp | ||
45 ↗ | (On Diff #214476) | 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? |
clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-register-over-unsigned.rst | ||
10 ↗ | (On Diff #214476) | It was meant to be 'initializer'. I'll correct that |
clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp | ||
60 ↗ | (On Diff #214476) | Added tests for the following cases that do not make changes*
*4, 6, and 8 should eventually but don't yet |
clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp | ||
---|---|---|
26 ↗ | (On Diff #214744) | I would rather use the fully-qualified names below -- the namespaces are actually of interest when needing to see what checks rely on what other modules quickly. |
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp | ||
45 ↗ | (On Diff #214476) | You could check if its DeclContext is the translation unit, I believe. |
clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp | ||
---|---|---|
26 ↗ | (On Diff #214744) | In that case I think I need some more detail on the alphabetical order I need to preserve. Do the namespaces factor into the order? If so, then PreferIsaOrDynCastInConditionalsCheck was in the wrong place prior to this patch and add_new_check.py is behaving correctly. If not, then the existing order was correct but add_new_check.py is inserting new lines incorrectly |
clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp | ||
---|---|---|
26 ↗ | (On Diff #214744) | We usually alphabetize based on the string literal for the check being registered. This usually results in the list also being sorted by the check implementation class name, but not always. |
Aside from the few remaining nits, I think this is almost ready to go.
clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp | ||
---|---|---|
32–33 ↗ | (On Diff #216290) | Spurious change that moved llvm-namespace-comment to be out of alphabetical order? |
clang-tools-extra/test/clang-tidy/llvm-prefer-register-over-unsigned.cpp | ||
60 ↗ | (On Diff #214476) | Nice, thank you! |
- Sort order again :-)
- Avoid getQualifiedNameAsString() in favour of getName() and checking
clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp | ||
---|---|---|
32–33 ↗ | (On Diff #216290) | Oops, not sure what happened there. I think I rolled it back to one of the earlier revisions somehow |
LGTM aside from a nit with auto.
clang-tools-extra/clang-tidy/llvm/PreferRegisterOverUnsignedCheck.cpp | ||
---|---|---|
47 ↗ | (On Diff #218015) | Don't use const auto * here as the type is not spelled out in the initializer. |