[PATCH] D54141: [clang-tidy] add deduplication support for run-clang-tidy.py
Stephen Kelly via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Nov 6 08:56:58 PST 2018
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.
rCTE Clang Tools Extra
More information about the cfe-commits