search

ZedThree / clang-tidy-review

Create a pull request review based on clang-tidy warnings
MIT License
88 stars 44 forks source link

Turn fixits into suggestions #4

Closed ZedThree closed 2 years ago

ZedThree commented 3 years ago

Rather than trying to work out if a comment is really a fixit, maybe try getting clang-tidy to fix everything, and then take the diff of that and add as a suggestion?

vadi2 commented 3 years ago

That would be great :+1:

vadi2 commented 3 years ago

Just checking, any bandwidth to add this in?

ZedThree commented 3 years ago

Probably not for a few weeks I'm afraid

ZedThree commented 3 years ago

I think I've found an easy-ish way to do this with the --export-fixes option, which exports to YAML.

ZedThree commented 3 years ago

So, annoyingly, the export-fixes yaml file just gives character offsets into the file, and not line numbers. This makes it very easy to apply the fix, but less easy to turn it into a Github suggestion fixit.

One way to use the existing method would be to look for multi-line warning bodies where the second line is a '^' by itself. Then the third line is what needs to be inserted. This is quite easy to do.

But there's still a couple of issues to do with fixes spanning multiple lines. For example, a fix might also require including a header. It might not be possible to put a suggestion comment to fix that if the line is not in the PR diff. There's probably more or less elegant ways of handling that.

The other one being something like wrapping a statement in braces. The output of clang-tidy (which is what we currently parse), only shows the first brace, whereas the exported fixes file has both.

Sooo, a complete but complicated method might be:

  1. run clang-tidy with --export-fixes=<file>.yaml
  2. read this file into a dict but convert this to be {"file": {<line>: ...}} -- or better, make a lookup map like we do for the source file <--> diff line numbers
  3. in make_review, when we find a warning, look up the file/line in the structure from step 2
  4. check each replacement:
    1. if its offset corresponds to the warning line, if so make it a suggestion;
    2. if not, check if it's in the diff, then make it's own comment and suggestion;
    3. otherwise, add it as a normal code block to this comment with a note that it needs to be applied manually

Now I've thought this through and worked out how to construct the lookup map in a non-terrible way, I will have a stab at doing it this weekend.