subject:"\[PATCH\] D116833\: \[clang\] Introduce support for disabling warnings in system macros"

[PATCH] D116833: [clang] Introduce support for disabling warnings in system macros

2022-01-12 Thread Carlos Galvez via Phabricator via cfe-commits

carlosgalvezp added a comment.

In D116833#3236209 , @rsmith wrote:

> Thanks, this looks nice.
>
> I think we'll need to think carefully before changing the default here. It 
> seems like the choice here would depend on what token the location of the 
> diagnostic points to -- if we know that the token is directly responsible for 
> the warning, then suppressing the warning makes sense, but if some of the 
> code responsible for the warning is outside the system header (even though 
> the token at the diagnostic location is not), then we probably still want to 
> warn. I don't think we provide enough information to the diagnostic system to 
> decide this on a global basis. In any case, this change should make it really 
> easy to give the new behavior to more diagnostics.

Thanks a lot for the quick review! I agree that we should analyze it carefully 
if we want to change default behavior. For now this solves my immediate needs 
so I'll leave it at that :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116833

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

[PATCH] D116833: [clang] Introduce support for disabling warnings in system macros

2022-01-12 Thread Carlos Galvez via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc4db521cea32: [clang] Introduce support for disabling 
warnings in system macros (authored by carlosgalvezp).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116833

Files:
  clang-tools-extra/clangd/Diagnostics.cpp
  clang/include/clang/Basic/Diagnostic.td
  clang/include/clang/Basic/DiagnosticAST.h
  clang/include/clang/Basic/DiagnosticAnalysis.h
  clang/include/clang/Basic/DiagnosticComment.h
  clang/include/clang/Basic/DiagnosticCrossTU.h
  clang/include/clang/Basic/DiagnosticDriver.h
  clang/include/clang/Basic/DiagnosticFrontend.h
  clang/include/clang/Basic/DiagnosticIDs.h
  clang/include/clang/Basic/DiagnosticLex.h
  clang/include/clang/Basic/DiagnosticParse.h
  clang/include/clang/Basic/DiagnosticRefactoring.h
  clang/include/clang/Basic/DiagnosticSema.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/DiagnosticSerialization.h
  clang/lib/Basic/DiagnosticIDs.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/warn-sysheader-macro.cpp
  clang/test/TableGen/DiagnosticBase.inc
  clang/test/TableGen/deferred-diag.td
  clang/tools/diagtool/DiagnosticNames.cpp
  clang/utils/TableGen/ClangDiagnosticsEmitter.cpp

Index: clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
===
--- clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
+++ clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
@@ -1311,6 +1311,11 @@
 else
   OS << ", false";
 
+if (R.getValueAsBit("ShowInSystemMacro"))
+  OS << ", true";
+else
+  OS << ", false";
+
 if (R.getValueAsBit("Deferrable"))
   OS << ", true";
 else
Index: clang/tools/diagtool/DiagnosticNames.cpp
===
--- clang/tools/diagtool/DiagnosticNames.cpp
+++ clang/tools/diagtool/DiagnosticNames.cpp
@@ -27,9 +27,9 @@
 // FIXME: Is it worth having two tables, especially when this one can get
 // out of sync easily?
 static const DiagnosticRecord BuiltinDiagnosticsByID[] = {
-#define DIAG(ENUM,CLASS,DEFAULT_MAPPING,DESC,GROUP,   \
- SFINAE,NOWERROR,SHOWINSYSHEADER,DEFER,CATEGORY)\
-  { #ENUM, diag::ENUM, STR_SIZE(#ENUM, uint8_t) },
+#define DIAG(ENUM, CLASS, DEFAULT_MAPPING, DESC, GROUP, SFINAE, NOWERROR,  \
+ SHOWINSYSHEADER, SHOWINSYSMACRO, DEFER, CATEGORY) \
+  {#ENUM, diag::ENUM, STR_SIZE(#ENUM, uint8_t)},
 #include "clang/Basic/DiagnosticCommonKinds.inc"
 #include "clang/Basic/DiagnosticCrossTUKinds.inc"
 #include "clang/Basic/DiagnosticDriverKinds.inc"
Index: clang/test/TableGen/deferred-diag.td
===
--- clang/test/TableGen/deferred-diag.td
+++ clang/test/TableGen/deferred-diag.td
@@ -5,23 +5,23 @@
 // Test usage of Deferrable and NonDeferrable in diagnostics.
 
 def test_default : Error<"This error is non-deferrable by default">;
-// CHECK-DAG: DIAG(test_default, {{.*}}SFINAE_SubstitutionFailure, false, true, false, 0)
+// CHECK-DAG: DIAG(test_default, {{.*}}SFINAE_SubstitutionFailure, false, true, true, false, 0)
 
 def test_deferrable : Error<"This error is deferrable">, Deferrable;
-// CHECK-DAG: DIAG(test_deferrable, {{.*}} SFINAE_SubstitutionFailure, false, true, true, 0)
+// CHECK-DAG: DIAG(test_deferrable, {{.*}} SFINAE_SubstitutionFailure, false, true, true, true, 0)
 
 def test_non_deferrable : Error<"This error is non-deferrable">, NonDeferrable;
-// CHECK-DAG: DIAG(test_non_deferrable, {{.*}} SFINAE_SubstitutionFailure, false, true, false, 0)
+// CHECK-DAG: DIAG(test_non_deferrable, {{.*}} SFINAE_SubstitutionFailure, false, true, true, false, 0)
 
 let Deferrable = 1 in {
 
 def test_let : Error<"This error is deferrable by let">;
-// CHECK-DAG: DIAG(test_let, {{.*}} SFINAE_SubstitutionFailure, false, true, true, 0)
+// CHECK-DAG: DIAG(test_let, {{.*}} SFINAE_SubstitutionFailure, false, true, true, true, 0)
 
 // Make sure TextSubstitution is allowed in the let Deferrable block.
 def textsub : TextSubstitution<"%select{text1|text2}0">;
 
 def test_let2 : Error<"This error is deferrable by let %sub{textsub}0">;
-// CHECK-DAG: DIAG(test_let2, {{.*}} SFINAE_SubstitutionFailure, false, true, true, 0)
+// CHECK-DAG: DIAG(test_let2, {{.*}} SFINAE_SubstitutionFailure, false, true, true, true, 0)
 
-}
\ No newline at end of file
+}
Index: clang/test/TableGen/DiagnosticBase.inc
===
--- clang/test/TableGen/DiagnosticBase.inc
+++ clang/test/TableGen/DiagnosticBase.inc
@@ -76,6 +76,7 @@
   bitAccessControl = 0;
   bitWarningNoWerror = 0;
   bitShowInSystemHeader = 0;
+  bitShowInSystemMacro = 1;
   bitDeferrable = 0;
   Severity

[PATCH] D116833: [clang] Introduce support for disabling warnings in system macros

2022-01-11 Thread Richard Smith - zygoloid via Phabricator via cfe-commits

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

Thanks, this looks nice.

I think we'll need to think carefully before changing the default here. It 
seems like the choice here would depend on what token the location of the 
diagnostic points to -- if we know that the token is directly responsible for 
the warning, then suppressing the warning makes sense, but if some of the code 
responsible for the warning is outside the system header (even though the token 
at the diagnostic location is not), then we probably still want to warn. I 
don't think we provide enough information to the diagnostic system to decide 
this on a global basis. In any case, this change should make it really easy to 
give the new behavior to more diagnostics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116833

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

[PATCH] D116833: [clang] Introduce support for disabling warnings in system macros

2022-01-10 Thread Carlos Galvez via Phabricator via cfe-commits

carlosgalvezp added a comment.

In D116833#3232739 , @efriedma wrote:

> I'll just note here that doing this globally is likely to have unexpected 
> results... consider, for example:
>
>   #include 
>   void f() { long x = M_PI; }
>
> Currently, the implicit conversion warning points into math.h.
>
> That said, I don't see any problem with the current implementation.

Yes, at first I set the default to "disable globally" and got around ~15 failed 
tests similar to your example. I don't have a good enough picture of all the 
existing warnings to determine what's preferred here, but I think we can take 
it in a separate patch. I get a much better insight now about why this has not 
been tackled before and why some warnings from system macros are needed!

I can also mention that we have recently merged a patch to disable clang-tidy 
warnings from system macros:
https://reviews.llvm.org/D116378

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116833

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

[PATCH] D116833: [clang] Introduce support for disabling warnings in system macros

2022-01-10 Thread Eli Friedman via Phabricator via cfe-commits

efriedma added a comment.

I'll just note here that doing this globally is likely to have unexpected 
results... consider, for example:

  #include 
  void f() { long x = M_PI; }

Currently, the implicit conversion warning points into math.h.

That said, I don't see any problem with the current implementation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116833

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

[PATCH] D116833: [clang] Introduce support for disabling warnings in system macros

2022-01-08 Thread Carlos Galvez via Phabricator via cfe-commits

carlosgalvezp updated this revision to Diff 398339.
carlosgalvezp retitled this revision from "[clang][Sema] Introduce support for 
disabling warnings in system macros" to "[clang] Introduce support for 
disabling warnings in system macros".
carlosgalvezp added a comment.
Herald added a subscriber: kadircet.
Herald added a project: clang-tools-extra.

Update DIAG in clang-tools-extra/Diagnostics


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116833

Files:
  clang-tools-extra/clangd/Diagnostics.cpp
  clang/include/clang/Basic/Diagnostic.td
  clang/include/clang/Basic/DiagnosticAST.h
  clang/include/clang/Basic/DiagnosticAnalysis.h
  clang/include/clang/Basic/DiagnosticComment.h
  clang/include/clang/Basic/DiagnosticCrossTU.h
  clang/include/clang/Basic/DiagnosticDriver.h
  clang/include/clang/Basic/DiagnosticFrontend.h
  clang/include/clang/Basic/DiagnosticIDs.h
  clang/include/clang/Basic/DiagnosticLex.h
  clang/include/clang/Basic/DiagnosticParse.h
  clang/include/clang/Basic/DiagnosticRefactoring.h
  clang/include/clang/Basic/DiagnosticSema.h
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/include/clang/Basic/DiagnosticSerialization.h
  clang/lib/Basic/DiagnosticIDs.cpp
  clang/lib/Sema/SemaDecl.cpp
  clang/lib/Sema/SemaExpr.cpp
  clang/test/SemaCXX/warn-sysheader-macro.cpp
  clang/test/TableGen/DiagnosticBase.inc
  clang/test/TableGen/deferred-diag.td
  clang/tools/diagtool/DiagnosticNames.cpp
  clang/utils/TableGen/ClangDiagnosticsEmitter.cpp

Index: clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
===
--- clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
+++ clang/utils/TableGen/ClangDiagnosticsEmitter.cpp
@@ -1311,6 +1311,11 @@
 else
   OS << ", false";
 
+if (R.getValueAsBit("ShowInSystemMacro"))
+  OS << ", true";
+else
+  OS << ", false";
+
 if (R.getValueAsBit("Deferrable"))
   OS << ", true";
 else
Index: clang/tools/diagtool/DiagnosticNames.cpp
===
--- clang/tools/diagtool/DiagnosticNames.cpp
+++ clang/tools/diagtool/DiagnosticNames.cpp
@@ -27,9 +27,9 @@
 // FIXME: Is it worth having two tables, especially when this one can get
 // out of sync easily?
 static const DiagnosticRecord BuiltinDiagnosticsByID[] = {
-#define DIAG(ENUM,CLASS,DEFAULT_MAPPING,DESC,GROUP,   \
- SFINAE,NOWERROR,SHOWINSYSHEADER,DEFER,CATEGORY)\
-  { #ENUM, diag::ENUM, STR_SIZE(#ENUM, uint8_t) },
+#define DIAG(ENUM, CLASS, DEFAULT_MAPPING, DESC, GROUP, SFINAE, NOWERROR,  \
+ SHOWINSYSHEADER, SHOWINSYSMACRO, DEFER, CATEGORY) \
+  {#ENUM, diag::ENUM, STR_SIZE(#ENUM, uint8_t)},
 #include "clang/Basic/DiagnosticCommonKinds.inc"
 #include "clang/Basic/DiagnosticCrossTUKinds.inc"
 #include "clang/Basic/DiagnosticDriverKinds.inc"
Index: clang/test/TableGen/deferred-diag.td
===
--- clang/test/TableGen/deferred-diag.td
+++ clang/test/TableGen/deferred-diag.td
@@ -5,23 +5,23 @@
 // Test usage of Deferrable and NonDeferrable in diagnostics.
 
 def test_default : Error<"This error is non-deferrable by default">;
-// CHECK-DAG: DIAG(test_default, {{.*}}SFINAE_SubstitutionFailure, false, true, false, 0)
+// CHECK-DAG: DIAG(test_default, {{.*}}SFINAE_SubstitutionFailure, false, true, true, false, 0)
 
 def test_deferrable : Error<"This error is deferrable">, Deferrable;
-// CHECK-DAG: DIAG(test_deferrable, {{.*}} SFINAE_SubstitutionFailure, false, true, true, 0)
+// CHECK-DAG: DIAG(test_deferrable, {{.*}} SFINAE_SubstitutionFailure, false, true, true, true, 0)
 
 def test_non_deferrable : Error<"This error is non-deferrable">, NonDeferrable;
-// CHECK-DAG: DIAG(test_non_deferrable, {{.*}} SFINAE_SubstitutionFailure, false, true, false, 0)
+// CHECK-DAG: DIAG(test_non_deferrable, {{.*}} SFINAE_SubstitutionFailure, false, true, true, false, 0)
 
 let Deferrable = 1 in {
 
 def test_let : Error<"This error is deferrable by let">;
-// CHECK-DAG: DIAG(test_let, {{.*}} SFINAE_SubstitutionFailure, false, true, true, 0)
+// CHECK-DAG: DIAG(test_let, {{.*}} SFINAE_SubstitutionFailure, false, true, true, true, 0)
 
 // Make sure TextSubstitution is allowed in the let Deferrable block.
 def textsub : TextSubstitution<"%select{text1|text2}0">;
 
 def test_let2 : Error<"This error is deferrable by let %sub{textsub}0">;
-// CHECK-DAG: DIAG(test_let2, {{.*}} SFINAE_SubstitutionFailure, false, true, true, 0)
+// CHECK-DAG: DIAG(test_let2, {{.*}} SFINAE_SubstitutionFailure, false, true, true, true, 0)
 
-}
\ No newline at end of file
+}
Index: clang/test/TableGen/DiagnosticBase.inc
===
--- clang/test/TableGen/DiagnosticBase.inc
+++ clang/test/TableGen/DiagnosticBase.inc
@@ -76,6 +76,7 @@
   bit

[PATCH] D116833: [clang] Introduce support for disabling warnings in system macros

[PATCH] D116833: [clang] Introduce support for disabling warnings in system macros

[PATCH] D116833: [clang] Introduce support for disabling warnings in system macros

[PATCH] D116833: [clang] Introduce support for disabling warnings in system macros

[PATCH] D116833: [clang] Introduce support for disabling warnings in system macros

[PATCH] D116833: [clang] Introduce support for disabling warnings in system macros

6 matches

Site Navigation

Mail list logo

Footer information