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

Wed Aug 7 08:12:50 PDT 2019

alexfh added a comment.

In D65065#1617031 <https://reviews.llvm.org/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 still report the issue. So he or she has to disable both (or as many aliases it has).
>
> That seems to be the case regardless of the implementation strategy in this patch.

The difference is that without the patch the user will initially see only one of the warnings, but after disabling the corresponding check the other warning will start appearing. Thus, the behavior with more granular deduplication results in a more consistent and less surprising experience.

As noted earlier, warning deduplication can hide different issues in checks that would usually better be fixed in the check itself. For example, the google-explicit-constructor check used in the test for deduplication (https://reviews.llvm.org/D2989) has been fixed since then as were a number of other checks.

================
Comment at: clang-tools-extra/test/clang-tidy/duplicate-reports.cpp:6-7
+  // At this location both cert-err09-cpp and cert-err61-cpp report the
+  // message. The order of the reports is unknown, that's why the checker name
+  // is not included in the expected warning.
+
----------------
I believe, this is not true anymore. Including the check name into the comparison key should make the order deterministic.

Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65065/new/

https://reviews.llvm.org/D65065