[PATCH] D38171: [clang-tidy] Implement clang-tidy check aliases

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: test/clang-tidy/misc-suspicious-semicolon-fail.cpp:5
-// RUN: clang-tidy %s -checks="-*,misc-suspicious-semicolon,clang-diagnostic*" 
\
-// RUN:-- -DWERROR -Wno-everything -Werror=unused-variable 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=CHECK-WERROR \

This reminds me that we should also filter out all -W(no-)error.* compiler 
arguments, if we're to take full control of clang diagnostics and make them 
behave closer to native checks.



Comment at: test/clang-tidy/validate-check-names.cpp:2
 // Check names may only contain alphanumeric characters, '-', '_', and '.'.
-// RUN: clang-tidy -checks=* -list-checks | grep '^' | cut -b5- | not grep 
-v '^[a-zA-Z0-9_.\-]\+$'
+// RUN: clang-tidy -checks=*,-clang-diagnostic* -list-checks | grep '^' | 
cut -b5- | not grep -v '^[a-zA-Z0-9_.\-]\+$'

Which clang diagnostic names fail this test?


https://reviews.llvm.org/D38171



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

[PATCH] D38171: [clang-tidy] Implement clang-tidy check aliases

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D38171#1036893, @alexfh wrote:

> I'm totally fine taking full control of clang-diagnostic- "checks", i.e. 
> automatically adding corresponding -W flags and removing all other -W flags 
> (or prepending -Wno-everything).
>
> However, could we split the aliases stuff to a separate patch?


The other way round. It will be better to split the clang-diagnostic part to a 
separate patch, since this patch focuses on the aliases.


https://reviews.llvm.org/D38171



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

[PATCH] D38171: [clang-tidy] Implement clang-tidy check aliases

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

I apologize for completely ignoring this for a long time.

I'm totally fine taking full control of clang-diagnostic- "checks", i.e. 
automatically adding corresponding -W flags and removing all other -W flags (or 
prepending -Wno-everything).

However, could we split the aliases stuff to a separate patch? My concerns 
w.r.t. the current implementation:

1. it doesn't allow more than one alias to each diagnostic
2. it's not clear that clang diagnostic aliases need to be separate from 
aliases for native clang-tidy checks. It might be better to add a more 
high-level API to register aliases (e.g. `registerCheckAlias(, 
, )`).




Comment at: clang-tidy/ClangTidy.cpp:406
+  for (const auto  : Context.getEnabledClangDiagnostics())
+CheckNames.push_back("clang-diagnostic-" + Diag);
+

The "clang-diagnostic-" string is now repeated multiple times. Could you pull 
it out to a constant (or maybe `llvm::StringRef getClangDiagnosticPrefix()`)?



Comment at: clang-tidy/ClangTidyModule.h:96
+  virtual void addWarningCheckAliases(
+  llvm::DenseMap ) {}
+

Consider using `llvm::StringMap`.


https://reviews.llvm.org/D38171



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

[PATCH] D38171: [clang-tidy] Implement clang-tidy check aliases

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added a comment.

While i don't have a leg to stand on here, i'd be *much* more comfortable if 
this would be a proper RFC mail in cfe-dev,
that would explore all the possible options (this, and the one from cfe-dev 
thread clang-tidy and CppCoreGuidelines 
),
and *explain* why one is chosen, and not the other one.

I still feel like that proposal is more sound.


https://reviews.llvm.org/D38171



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

[PATCH] D38171: [clang-tidy] Implement clang-tidy check aliases

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added subscribers: dkrupp, whisperity.
xazax.hun added a comment.

@alexfh did you have any chance to think about this change?


https://reviews.llvm.org/D38171



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

[PATCH] D38171: [clang-tidy] Implement clang-tidy check aliases

[András Leitereg via Phabricator via cfe-commits]

leanil marked 2 inline comments as done.
leanil added a comment.

Does anyone have any more thoughts about this?


https://reviews.llvm.org/D38171



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

[PATCH] D38171: [clang-tidy] Implement clang-tidy check aliases

[András Leitereg via Phabricator via cfe-commits]

leanil updated this revision to Diff 127736.
leanil added a comment.

Remove redundant empty lines.
Make list-clang-diagnostics test less strict.
Update getAllDiagnostics to use std::vector.


https://reviews.llvm.org/D38171

Files:
  clang-tidy/ClangTidy.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tidy/ClangTidyDiagnosticConsumer.h
  clang-tidy/ClangTidyModule.h
  clang-tidy/cert/CERTTidyModule.cpp
  clang-tidy/tool/ClangTidyMain.cpp
  test/clang-tidy/cert-exp59-cpp.cpp
  test/clang-tidy/custom-diagnostics.cpp
  test/clang-tidy/diagnostic.cpp
  test/clang-tidy/list-clang-diagnostics.cpp
  test/clang-tidy/misc-suspicious-semicolon-fail.cpp
  test/clang-tidy/validate-check-names.cpp
  test/clang-tidy/warning-check-aliases.cpp
  test/clang-tidy/werrors-diagnostics.cpp

Index: test/clang-tidy/werrors-diagnostics.cpp
===
--- test/clang-tidy/werrors-diagnostics.cpp
+++ test/clang-tidy/werrors-diagnostics.cpp
@@ -1,11 +1,10 @@
-// RUN: clang-tidy %s -checks='-*,llvm-namespace-comment,clang-diagnostic*' \
-// RUN:   -- -Wunused-variable 2>&1 \
+// RUN: clang-tidy %s -checks='-*,llvm-namespace-comment,clang-diagnostic-unused-variable' -- 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-WARN -implicit-check-not='{{warning|error}}:'
-// RUN: not clang-tidy %s -checks='-*,llvm-namespace-comment,clang-diagnostic*' \
-// RUN:   -warnings-as-errors='clang-diagnostic*' -- -Wunused-variable 2>&1 \
+// RUN: not clang-tidy %s -checks='-*,llvm-namespace-comment,clang-diagnostic-unused-variable' \
+// RUN:   -warnings-as-errors='clang-diagnostic*' -- 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-WERR -implicit-check-not='{{warning|error}}:'
-// RUN: not clang-tidy %s -checks='-*,llvm-namespace-comment,clang-diagnostic*' \
-// RUN:   -warnings-as-errors='clang-diagnostic*' -quiet -- -Wunused-variable 2>&1 \
+// RUN: not clang-tidy %s -checks='-*,llvm-namespace-comment,clang-diagnostic-unused-variable' \
+// RUN:   -warnings-as-errors='clang-diagnostic*' -quiet -- 2>&1 \
 // RUN:   | FileCheck %s --check-prefix=CHECK-WERR-QUIET -implicit-check-not='{{warning|error}}:'
 
 void f() { int i; }
Index: test/clang-tidy/warning-check-aliases.cpp
===
--- /dev/null
+++ test/clang-tidy/warning-check-aliases.cpp
@@ -0,0 +1,19 @@
+// RUN: clang-tidy %s -checks='-*,clang-diagnostic-exceptions' -- 2>&1 | FileCheck -implicit-check-not='{{warning:|error:}}' %s
+// RUN: clang-tidy %s -checks='-*,cert-err54-cpp' -- 2>&1 | FileCheck -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK2 %s
+// RUN: clang-tidy %s -- 2>&1 | FileCheck -allow-empty -implicit-check-not='{{warning:|error:}}' -check-prefix=CHECK3 %s
+
+class B {};
+class D : public B {};
+
+void f() {
+  try {
+  } catch (B ) {
+  } catch (D ) {
+  }
+}
+
+//CHECK: :11:12: warning: exception of type 'D &' will be caught by earlier handler [clang-diagnostic-exceptions]
+//CHECK: :10:12: note: for type 'B &'
+
+//CHECK2: :11:12: warning: exception of type 'D &' will be caught by earlier handler [cert-err54-cpp]
+//CHECK2: :10:12: note: for type 'B &'
Index: test/clang-tidy/validate-check-names.cpp
===
--- test/clang-tidy/validate-check-names.cpp
+++ test/clang-tidy/validate-check-names.cpp
@@ -1,2 +1,2 @@
 // Check names may only contain alphanumeric characters, '-', '_', and '.'.
-// RUN: clang-tidy -checks=* -list-checks | grep '^' | cut -b5- | not grep -v '^[a-zA-Z0-9_.\-]\+$'
+// RUN: clang-tidy -checks=*,-clang-diagnostic* -list-checks | grep '^' | cut -b5- | not grep -v '^[a-zA-Z0-9_.\-]\+$'
Index: test/clang-tidy/misc-suspicious-semicolon-fail.cpp
===
--- test/clang-tidy/misc-suspicious-semicolon-fail.cpp
+++ test/clang-tidy/misc-suspicious-semicolon-fail.cpp
@@ -1,8 +1,8 @@
 // RUN: clang-tidy %s -checks="-*,misc-suspicious-semicolon" -- -DERROR 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=CHECK-ERROR \
 // RUN:   -implicit-check-not="{{warning|error}}:"
-// RUN: clang-tidy %s -checks="-*,misc-suspicious-semicolon,clang-diagnostic*" \
-// RUN:-- -DWERROR -Wno-everything -Werror=unused-variable 2>&1 \
+// RUN: not clang-tidy %s -checks="-*,misc-suspicious-semicolon,clang-diagnostic-unused-variable" \
+// RUN:   -warnings-as-errors=clang-diagnostic-unused-variable -- -DWERROR 2>&1 \
 // RUN:   | FileCheck %s -check-prefix=CHECK-WERROR \
 // RUN:   -implicit-check-not="{{warning|error}}:"
 
@@ -19,7 +19,7 @@
   // CHECK-ERROR: :[[@LINE-1]]:8: error: expected ';' at end of declaration [clang-diagnostic-error]
 #elif WERROR
   int a;
-  // CHECK-WERROR: :[[@LINE-1]]:7: error: unused variable 'a' [clang-diagnostic-unused-variable]
+  // CHECK-WERROR: :[[@LINE-1]]:7: error: unused variable 'a' [clang-diagnostic-unused-variable,-warnings-as-errors]
 #else
 #error "One of ERROR or 

[PATCH] D38171: [clang-tidy] Implement clang-tidy check aliases

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D38171#927046, @alexfh wrote:

> And, btw, sorry for the long delay. I've been on travelling / on vacation for 
> the last few weeks.


No problem. Will you have some time to think about the overall concept? 
Implementation and test wise it looks good to me.
I think this patch is a step in a good direction but of course, it is important 
for the community to agree on the approach.




Comment at: test/clang-tidy/warning-check-aliases.cpp:10
+  try {
+
+  } catch (B ) {

There are some redundant empty lines. 


https://reviews.llvm.org/D38171



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

[PATCH] D38171: [clang-tidy] Implement clang-tidy check aliases

[Malcolm Parsons via Phabricator via cfe-commits]

malcolm.parsons added a comment.

In https://reviews.llvm.org/D38171#927045, @alexfh wrote:

> The only place I can think of, where -checks=* is useful is in combination 
> with -list-checks, where the presence of clang-diagnostic- entries would be 
> desired anyway.


Can we replace -list-checks with -list-enabled-checks and -list-all-checks?  
The current behaviour is confusing and recently caused Chandler to abandon a 
clang-tidy demo.


https://reviews.llvm.org/D38171



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

[PATCH] D38171: [clang-tidy] Implement clang-tidy check aliases

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

And, btw, sorry for the long delay. I've been on travelling / on vacation for 
the last few weeks.


https://reviews.llvm.org/D38171



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

[PATCH] D38171: [clang-tidy] Implement clang-tidy check aliases

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D38171#925929, @xazax.hun wrote:

> In https://reviews.llvm.org/D38171#909346, @leanil wrote:
>
> > In https://reviews.llvm.org/D38171#901427, @xazax.hun wrote:
> >
> > > One problem to think about when we add all clang-diagnostic as "first or 
> > > second" class citizen, `checkes=*` might now enable all the warnings 
> > > which make no sense and might be surprising to the users. What do you 
> > > think?
> >
> >
> > This is a good point. Should I insert ",-clang-diagnostic*" after any 
> > (positive) * ?
>
>
> @alexfh do you have some thoughts on this?


I don't think this particular point is a large concern. As I mentioned multiple 
times, enabling all checks is almost never useful due to many checks 
overlapping or producing conflicting advice. The only place I can think of, 
where -checks=* is useful is in combination with -list-checks, where the 
presence of clang-diagnostic- entries would be desired anyway.


https://reviews.llvm.org/D38171



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

[PATCH] D38171: [clang-tidy] Implement clang-tidy check aliases

[Gábor Horváth via Phabricator via cfe-commits]

xazax.hun added a comment.

In https://reviews.llvm.org/D38171#909346, @leanil wrote:

> In https://reviews.llvm.org/D38171#901427, @xazax.hun wrote:
>
> > One problem to think about when we add all clang-diagnostic as "first or 
> > second" class citizen, `checkes=*` might now enable all the warnings which 
> > make no sense and might be surprising to the users. What do you think?
>
>
> This is a good point. Should I insert ",-clang-diagnostic*" after any 
> (positive) * ?


@alexfh do you have some thoughts on this?


https://reviews.llvm.org/D38171



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