Page MenuHomePhabricator

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

Authored by dsanders on Aug 7 2019, 6:40 PM.

Details

Summary

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).

Diff Detail

Repository
rL LLVM

Event Timeline

dsanders created this revision.Aug 7 2019, 6:40 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 7 2019, 6:40 PM
lebedev.ri retitled this revision from Add llvm-prefer-register-over-unsigned to clang-tidy and apply it to LLVM to [clang-tidy] Add llvm-prefer-register-over-unsigned check and apply it to LLVM.Aug 8 2019, 5:02 AM

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.

dsanders updated this revision to Diff 214194.Aug 8 2019, 11:05 AM
dsanders marked 2 inline comments as done.

UsingDirectiveDecl -> auto
-> `
Split LLVM changes into another patch

dsanders edited the summary of this revision. (Show Details)Aug 8 2019, 11:06 AM
Eugene.Zelenko removed a project: Restricted Project.
Eugene.Zelenko removed subscribers: wuzish, jholewinski, MatzeB and 43 others.

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.

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.

aaron.ballman added inline comments.Aug 8 2019, 1:48 PM
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.

dsanders marked an inline comment as done.Aug 8 2019, 3:48 PM
dsanders added inline comments.
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?

dsanders updated this revision to Diff 214276.Aug 8 2019, 6:56 PM

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

dsanders marked 4 inline comments as done.Aug 8 2019, 7:00 PM
dsanders added inline comments.
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.

aaron.ballman added inline comments.Aug 9 2019, 4:49 AM
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.

dsanders updated this revision to Diff 214476.Aug 9 2019, 4:08 PM
  • 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
dsanders marked 3 inline comments as done.Aug 9 2019, 4:13 PM
dsanders added inline comments.
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

aaron.ballman marked an inline comment as done.Aug 10 2019, 8:44 AM
aaron.ballman added inline comments.
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
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.)

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.

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();
}
dsanders updated this revision to Diff 214744.Aug 12 2019, 4:57 PM
dsanders marked 9 inline comments as done.
  • 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*

  1. Similar interface but not called Register
  2. Register class not inside llvm namespace
  3. Calling a function that takes an llvm::Register with a llvm::Register argument
  4. Calling a function that takes an unsigned and is given an llvm::Register argument
  5. Initializing an llvm::Register using an llvm::Register argument
  6. Initializing any other object using a constructor parameter that's unsigned and a llvm::Register argument
  7. Assigning to a member of llvm::Register
  8. Assigning to a unsigned member of any other object

*4, 6, and 8 should eventually but don't yet

aaron.ballman added inline comments.Aug 13 2019, 5:44 AM
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.

dsanders marked an inline comment as done.Aug 13 2019, 4:52 PM
dsanders added inline comments.
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

aaron.ballman added inline comments.Aug 14 2019, 8:26 AM
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.

bjope added a subscriber: bjope.Aug 16 2019, 12:51 PM
dsanders updated this revision to Diff 216290.Aug 20 2019, 4:47 PM
dsanders marked 2 inline comments as done.
  • registerCheck<> order

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!

dsanders updated this revision to Diff 218015.Aug 29 2019, 8:04 PM
dsanders marked 4 inline comments as done.
  • Sort order again :-)
  • Avoid getQualifiedNameAsString() in favour of getName() and checking
dsanders added inline comments.Aug 29 2019, 8:05 PM
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

aaron.ballman accepted this revision.Aug 30 2019, 4:56 AM

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.

This revision is now accepted and ready to land.Aug 30 2019, 4:56 AM
This revision was automatically updated to reflect the committed changes.
dsanders marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptAug 30 2019, 1:01 PM