[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

This revision was automatically updated to reflect the committed changes. Closed by commit rL369763: [clang-tidy] Possibility of displaying duplicate warnings (authored by Szelethus, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

bruntib added a comment. Thank you for the valuable comments and the review! I'm just planning to register to the bug tracker, because it is getting more relevant due to some other contributions as well. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

alexfh accepted this revision. alexfh added a comment. This revision is now accepted and ready to land. LG. Thanks! Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65065/new/ https://reviews.llvm.org/D65065

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

alexfh added a comment. Just FYI, https://bugs.llvm.org/show_bug.cgi?id=43019 is relevant. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65065/new/ https://reviews.llvm.org/D65065 ___ cfe-commits

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

Szelethus added a comment. I have no authority to accept patches in clang-tidy (though please feel free to add me as a reviewer, I can more easily participate in the discussion!), this looks good to me too, thanks! Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

lebedev.ri added a comment. LGTM Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65065/new/ https://reviews.llvm.org/D65065 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

bruntib updated this revision to Diff 215122. bruntib added a comment. Since now the order of diagnostic messages is deterministic, we can count on this order in the test file. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65065/new/

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

alexfh added a comment. In D65065#1617031 , @gribozavr wrote: > > This suggestion would result another strange behavior: if the user disables > > cert-err09-cpp because he or she doesn't want to see its reports, the other > > one (cert-err61-cpp) will

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

lebedev.ri added a comment. In D65065#1617079 , @gribozavr wrote: > `-Weverything` is not recommended for anything except compiler testing, and > similarly with clang-tidy, enabling all checkers is not a good idea. I don't > think improving this

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

gribozavr added a comment. `-Weverything` is not recommended for anything except compiler testing, and similarly with clang-tidy, enabling all checkers is not a good idea. I don't think improving this particular point will make enabling all checks more usable. Repository: rCTE Clang Tools

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

lebedev.ri added a comment. In D65065#1617031 , @gribozavr wrote: > > This suggestion would result another strange behavior: if the user disables > > cert-err09-cpp because he or she doesn't want to see its reports, the other > > one (cert-err61-cpp)

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

gribozavr added a comment. > This suggestion would result another strange behavior: if the user disables > cert-err09-cpp because he or she doesn't want to see its reports, the other > one (cert-err61-cpp) will still report the issue. So he or she has to disable > both (or as many aliases it

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

bruntib added a comment. This suggestion would result another strange behavior: if the user disables cert-err09-cpp because he or she doesn't want to see its reports, the other one (cert-err61-cpp) will still report the issue. So he or she has to disable both (or as many aliases it has). As

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

gribozavr added a comment. Producing the message two times is worse user experience than producing one. Most users don't care which checker produced the message. However, the output should be deterministic. Therefore, a better fix would be making deduplication deterministic, instead of

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

bruntib added a comment. Not exactly. The problem is that it is non-deterministic which checker reports the issue. For example before this patch, in the test file above there was only one report. However, sometimes the report message is: throw expression should throw anonymous temporary values

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

gribozavr added a comment. So the problem you're trying to solve is that the output is non-deterministic, is that correct? > However, it is random which checker name is included in the warning. If that is indeed the problem, then I think the solution should be making the output deterministic

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

bruntib added a comment. Alright, I modified the commit accordingly. Thank you for the suggestions. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65065/new/ https://reviews.llvm.org/D65065 ___

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

bruntib updated this revision to Diff 213356. bruntib edited the summary of this revision. Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65065/new/ https://reviews.llvm.org/D65065 Files: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

alexfh added a comment. I think it will be a strict improvement to include the check name into the deduplication key (probably after the file and offset and before the message). I don't see any reason to hide this behind a flag or otherwise retain the old behavior. As for expanding the key to

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

bruntib added a comment. Yes, LessClangTidyError would really be the best place for this. The reason of my implementation was that I wanted to bind this feature to a command line option. I don't know if there was any design decision behind the current uniquing logic and I didn't want to break

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

alexfh added a comment. LessClangTidyError only compares location and message, but it could also compare other things like notes, fixes, etc. For the problem outlined in the description of this patch we can probably include the checker name into the key. WDYT? Repository: rCTE Clang Tools

[PATCH] D65065: [clang-tidy] Possibility of displaying duplicate warnings

bruntib created this revision. bruntib added reviewers: alexfh, xazax.hun, Szelethus. Herald added subscribers: cfe-commits, mgrang, rnkovacs, whisperity. Herald added a project: clang. In case a checker is registered multiple times as an alias, the emitted warnings are uniqued by the report