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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
