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

[Kristóf Umann via Phabricator via cfe-commits]

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:
  https://reviews.llvm.org/D65065?vs=215122=216849#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65065

Files:
  clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/trunk/test/clang-tidy/duplicate-reports.cpp


Index: clang-tools-extra/trunk/test/clang-tidy/duplicate-reports.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/duplicate-reports.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/duplicate-reports.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cert-err09-cpp,cert-err61-cpp %t
+
+void alwaysThrows() {
+  int ex = 42;
+  // CHECK-MESSAGES: warning: throw expression should throw anonymous 
temporary values instead [cert-err09-cpp]
+  // CHECK-MESSAGES: warning: throw expression should throw anonymous 
temporary values instead [cert-err61-cpp]
+  throw ex;
+}
+
+void doTheJob() {
+  try {
+alwaysThrows();
+  } catch (int&) {
+  }
+}
Index: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -742,8 +742,9 @@
 const tooling::DiagnosticMessage  = LHS.Message;
 const tooling::DiagnosticMessage  = RHS.Message;
 
-return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
-   std::tie(M2.FilePath, M2.FileOffset, M2.Message);
+return
+  std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, M1.Message) <
+  std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message);
   }
 };
 struct EqualClangTidyError {


Index: clang-tools-extra/trunk/test/clang-tidy/duplicate-reports.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/duplicate-reports.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/duplicate-reports.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cert-err09-cpp,cert-err61-cpp %t
+
+void alwaysThrows() {
+  int ex = 42;
+  // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead [cert-err09-cpp]
+  // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead [cert-err61-cpp]
+  throw ex;
+}
+
+void doTheJob() {
+  try {
+alwaysThrows();
+  } catch (int&) {
+  }
+}
Index: clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -742,8 +742,9 @@
 const tooling::DiagnosticMessage  = LHS.Message;
 const tooling::DiagnosticMessage  = RHS.Message;
 
-return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
-   std::tie(M2.FilePath, M2.FileOffset, M2.Message);
+return
+  std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, M1.Message) <
+  std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message);
   }
 };
 struct EqualClangTidyError {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tibor Brunner via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D65065/new/

https://reviews.llvm.org/D65065



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

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

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

[Kristóf Umann via Phabricator via cfe-commits]

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
  https://reviews.llvm.org/D65065/new/

https://reviews.llvm.org/D65065



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tibor Brunner via Phabricator via cfe-commits]

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/

https://reviews.llvm.org/D65065

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/test/clang-tidy/duplicate-reports.cpp


Index: clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cert-err09-cpp,cert-err61-cpp %t
+
+void alwaysThrows() {
+  int ex = 42;
+  // CHECK-MESSAGES: warning: throw expression should throw anonymous 
temporary values instead [cert-err09-cpp]
+  // CHECK-MESSAGES: warning: throw expression should throw anonymous 
temporary values instead [cert-err61-cpp]
+  throw ex;
+}
+
+void doTheJob() {
+  try {
+alwaysThrows();
+  } catch (int&) {
+  }
+}
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -742,8 +742,9 @@
 const tooling::DiagnosticMessage  = LHS.Message;
 const tooling::DiagnosticMessage  = RHS.Message;
 
-return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
-   std::tie(M2.FilePath, M2.FileOffset, M2.Message);
+return
+  std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, M1.Message) <
+  std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message);
   }
 };
 struct EqualClangTidyError {


Index: clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s cert-err09-cpp,cert-err61-cpp %t
+
+void alwaysThrows() {
+  int ex = 42;
+  // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead [cert-err09-cpp]
+  // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead [cert-err61-cpp]
+  throw ex;
+}
+
+void doTheJob() {
+  try {
+alwaysThrows();
+  } catch (int&) {
+  }
+}
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -742,8 +742,9 @@
 const tooling::DiagnosticMessage  = LHS.Message;
 const tooling::DiagnosticMessage  = RHS.Message;
 
-return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
-   std::tie(M2.FilePath, M2.FileOffset, M2.Message);
+return
+  std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, M1.Message) <
+  std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message);
   }
 };
 struct EqualClangTidyError {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

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 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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

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 particular point will make enabling all checks more 
> usable.


That is beyond the point here (and is very arguable).
Even just enabling all `cert-*` checks will result in this behavior.


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitri Gribenko via Phabricator via cfe-commits]

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 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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Roman Lebedev via Phabricator via cfe-commits]

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) 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.
>
> > This could be another advantage of this patch, since it would be annoying 
> > to see a message in which a GCC-style checker warns about an LLVM-style 
> > violation.
>
> I don't understand the scenario. I think people wouldn't enable both checkers 
> in the first place.


They can silently get enabled if you follow the -Weverything approach - enable 
all checks, and selectively disable.
Current clang-tidy check aliasing really looks misdesigned..


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitri Gribenko via Phabricator via cfe-commits]

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 has).

That seems to be the case regardless of the implementation strategy in this 
patch.

> This could be another advantage of this patch, since it would be annoying to 
> see a message in which a GCC-style checker warns about an LLVM-style 
> violation.

I don't understand the scenario. I think people wouldn't enable both checkers 
in the first place.


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tibor Brunner via Phabricator via cfe-commits]

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 far as I know the idea behind aliases is that a checker can be registered 
with different options on different names. For example a code style checker can 
be registered with different names (e.g. GCC-style, LLVM-style, etc.) with 
different checker options on code styles. This could be another advantage of 
this patch, since it would be annoying to see a message in which a GCC-style 
checker warns about an LLVM-style violation.


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitri Gribenko via Phabricator via cfe-commits]

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 printing the message twice.


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tibor Brunner via Phabricator via cfe-commits]

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 instead 
[cert-err09-cpp]

and sometimes the message is:

throw expression should throw anonymous temporary values instead 
[cert-err61-cpp]

(note the content of the square bracket)
So after this patch both of these lines will be emitted.


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Dmitri Gribenko via Phabricator via cfe-commits]

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 -- not printing the message multiple times...


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tibor Brunner via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tibor Brunner via Phabricator via cfe-commits]

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
  clang-tools-extra/test/clang-tidy/duplicate-reports.cpp


Index: clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy %s cert-err09-cpp,cert-err61-cpp %t
+
+void alwaysThrows() {
+  int ex = 42;
+  // 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.
+
+  // CHECK-MESSAGES: warning: throw expression should throw anonymous 
temporary values instead
+  // CHECK-MESSAGES: warning: throw expression should throw anonymous 
temporary values instead
+  throw ex;
+}
+
+void doTheJob() {
+  try {
+alwaysThrows();
+  } catch (int&) {
+  }
+}
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -742,8 +742,9 @@
 const tooling::DiagnosticMessage  = LHS.Message;
 const tooling::DiagnosticMessage  = RHS.Message;
 
-return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
-   std::tie(M2.FilePath, M2.FileOffset, M2.Message);
+return
+  std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, M1.Message) <
+  std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message);
   }
 };
 struct EqualClangTidyError {


Index: clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/duplicate-reports.cpp
@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy %s cert-err09-cpp,cert-err61-cpp %t
+
+void alwaysThrows() {
+  int ex = 42;
+  // 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.
+
+  // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead
+  // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead
+  throw ex;
+}
+
+void doTheJob() {
+  try {
+alwaysThrows();
+  } catch (int&) {
+  }
+}
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -742,8 +742,9 @@
 const tooling::DiagnosticMessage  = LHS.Message;
 const tooling::DiagnosticMessage  = RHS.Message;
 
-return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
-   std::tie(M2.FilePath, M2.FileOffset, M2.Message);
+return
+  std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, M1.Message) <
+  std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message);
   }
 };
 struct EqualClangTidyError {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

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 include notes and fixes - it's probably 
good to do this either, and this may help uncover incorrect behavior of some 
checks. I'd suggest to start with the check name though.


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tibor Brunner via Phabricator via cfe-commits]

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 it if there was any. So would you be 
agree with having duplicate reports by default without any command line flag in 
case of alias checkers? I believe this would be the most painless solution, 
since in case of further improvements about including notes, fixes, etc. in the 
functor, the potential command line option controlling this would be 
unnecessarily complex. I don't know if there could be any use-case on those.
Thanks for the review!


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexander Kornienko via Phabricator via cfe-commits]

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 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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Tibor Brunner via Phabricator via cfe-commits]

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 message. However, it is random which checker 
name is included in the warning. When processing the output of clang-tidy this 
behavior caused some problems. So in this commit clang-tidy has been extended 
with a --duplicate-reports flag which prevents uniquing the warnings and both 
checker's messages are printed under their name.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D65065

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
  clang-tools-extra/clang-tidy/ClangTidyOptions.h
  clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
  clang-tools-extra/test/clang-tidy/duplicate-reports-with.cpp
  clang-tools-extra/test/clang-tidy/duplicate-reports-without.cpp

Index: clang-tools-extra/test/clang-tidy/duplicate-reports-without.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/duplicate-reports-without.cpp
@@ -0,0 +1,17 @@
+// RUN: %check_clang_tidy %s cert-err09-cpp,cert-err61-cpp %t
+
+void alwaysThrows() {
+  int ex = 42;
+  // It is unknown which checker alias is reporting the warning, that is why
+  // the checker name is not included in the message.
+
+  // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead
+  throw ex;
+}
+
+void doTheJob() {
+  try {
+alwaysThrows();
+  } catch (int&) {
+  }
+}
Index: clang-tools-extra/test/clang-tidy/duplicate-reports-with.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/duplicate-reports-with.cpp
@@ -0,0 +1,19 @@
+// RUN: %check_clang_tidy %s cert-err09-cpp,cert-err61-cpp %t -duplicate-reports --
+
+void alwaysThrows() {
+  int ex = 42;
+  // 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.
+
+  // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead
+  // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead
+  throw ex;
+}
+
+void doTheJob() {
+  try {
+alwaysThrows();
+  } catch (int&) {
+  }
+}
Index: clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
===
--- clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -78,6 +78,15 @@
  cl::init(""),
  cl::cat(ClangTidyCategory));
 
+static cl::opt DuplicateReports("duplicate-reports", cl::desc(R"(
+Some checkers are aliases of others, so the same
+issue may be reported by different checkers at
+the same location. These duplicates are uniqued
+unless this flag is provided.
+)"),
+ cl::init(false),
+ cl::cat(ClangTidyCategory));
+
 static cl::opt HeaderFilter("header-filter", cl::desc(R"(
 Regular expression matching the names of the
 headers to output diagnostics from. Diagnostics
@@ -266,6 +275,7 @@
   ClangTidyOptions DefaultOptions;
   DefaultOptions.Checks = DefaultChecks;
   DefaultOptions.WarningsAsErrors = "";
+  DefaultOptions.DuplicateReports = DuplicateReports;
   DefaultOptions.HeaderFilterRegex = HeaderFilter;
   DefaultOptions.SystemHeaders = SystemHeaders;
   DefaultOptions.FormatStyle = FormatStyle;
@@ -279,6 +289,8 @@
 OverrideOptions.Checks = Checks;
   if (WarningsAsErrors.getNumOccurrences() > 0)
 OverrideOptions.WarningsAsErrors = WarningsAsErrors;
+  if (DuplicateReports.getNumOccurrences() > 0)
+OverrideOptions.DuplicateReports = DuplicateReports;
   if (HeaderFilter.getNumOccurrences() > 0)
 OverrideOptions.HeaderFilterRegex = HeaderFilter;
   if (SystemHeaders.getNumOccurrences() > 0)
Index: clang-tools-extra/clang-tidy/ClangTidyOptions.h
===
--- clang-tools-extra/clang-tidy/ClangTidyOptions.h
+++ clang-tools-extra/clang-tidy/ClangTidyOptions.h
@@ -66,6 +66,10 @@
   /// \brief WarningsAsErrors filter.
   llvm::Optional WarningsAsErrors;
 
+  /// \brief Indicates whether warnings emitted by alias checkers should be
+  /// reported multiple times.
+  llvm::Optional DuplicateReports;
+
   /// \brief Output warnings from headers matching this filter. Warnings from
   /// main files will always be displayed.
   llvm::Optional HeaderFilterRegex;
Index: clang-tools-extra/clang-tidy/ClangTidyOptions.cpp