[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

In https://reviews.llvm.org/D54141#1288818, @JonasToth wrote:

> Thank you for the comment!
>
> In https://reviews.llvm.org/D54141#1288809, @steveire wrote:
>
> > This feature seems like a good idea. I started writing it too some months ago, but then I changed tactic and worked on distributing the refactor over the network instead. As far as I know, your deduplication would not work with a distributed environment.
>
>
> I agree that it would probably not work. It might enable a two-stage deduplication, but I don't know if that would be viable.


Yes, I think the distributed refactoring would benefit from the design I outlined - issuing diagnostics from the yaml files.

> In principle this approach seems more robust and I am not claiming my approach is robust at all :)
>  The point hokein raised should be considered first in my opinion. If clang-tidy itself is already parallel we should definitely deduplicate there. This is something I would put more
>  effort in. The proposed solution is more a hack to get my buildbot running and find transformation bugs and provide real-world data for checks we implement. :)

Yes, I think it makes sense to do something more robust. I understand you're yak shaving here a bit while trying to reach a higher goal.

The AllTUsToolExecutor idea is worth exploring - it would mean we could remove the threading from run-clang-tidy.py.

I don't think we should get a self-confessed hack in just because it's already written.

However, AllTUsToolExecutor seems to not create output replacement files at all, which is not distributed-refactoring-friendly.

>> I think clang-apply-replacements already does de-duplication, so it's possible that could take more responsibility.
> 
> Yes, the emitted fixes are deduplicated but i think we need something even if no fixes are involved.

Maybe my suggestion was not clear. The yaml file generated by clang-tidy contains not only replacements, but all diagnostics, even without a fixit.

So, running `clang-apply-replacements --issue-diags the_new_file.yaml` would issue the warnings/fixit hints by processing the yaml and issuing the diagnostics the way clang-tidy would have done (See in my proposed design that we silence clang-tidy).

Note also that the `--issue-diags` option does not yet exist. I'm proposing adding it.

> 
> 
>> Also, I think your test content is too big. I suggest trying to write more contained tests for this.
> 
> I wanted to have a mix of both real snippets and some unit-tests on short examples. Do you think its enough if i shorten the list of fields that the CSA output contains for the padding checker?

It seems that the bulk of the testing part of this commit is parsing a real-world log that you made. I guess if you remove the parsing (by taking a machine-readable approach) that bulk will disappear anyway.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D54141