[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG992fa7be3438: [clang-tidy] Initialize DiagnosticEngine in
ExpandModularHeaders (authored by PiotrZSL).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156056/new/
https://reviews.llvm.org/D156056
Files:
clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
Index: clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
@@ -23,6 +23,8 @@
// RUN: clang-tidy
-checks='-*,modernize-use-override,clang-diagnostic-macro-redefined'
%T/diagnostics/input.cpp -- -DMACRO_FROM_COMMAND_LINE 2>&1 | FileCheck
-check-prefix=CHECK4 -implicit-check-not='{{warning:|error:}}' %s
// RUN: not clang-tidy
-checks='-*,clang-diagnostic-*,google-explicit-constructor'
%T/diagnostics/input.cpp 2>&1 | FileCheck -check-prefix=CHECK5
-implicit-check-not='{{warning:|error:}}' %s
// RUN: not clang-tidy -checks='-*,modernize-use-override'
%T/diagnostics/input.cpp -- -DCOMPILATION_ERROR 2>&1 | FileCheck
-check-prefix=CHECK6 -implicit-check-not='{{warning:|error:}}' %s
+// RUN: clang-tidy
-checks='-*,modernize-use-override,clang-diagnostic-macro-redefined' %s --
-DMACRO_FROM_COMMAND_LINE -std=c++20 | FileCheck -check-prefix=CHECK4
-implicit-check-not='{{warning:|error:}}' %s
+// RUN: clang-tidy
-checks='-*,modernize-use-override,clang-diagnostic-macro-redefined,clang-diagnostic-literal-conversion'
%s -- -DMACRO_FROM_COMMAND_LINE -std=c++20 -Wno-macro-redefined | FileCheck
--check-prefix=CHECK7 -implicit-check-not='{{warning:|error:}}' %s
// CHECK1: error: no input files [clang-diagnostic-error]
// CHECK1: error: no such file or directory: '{{.*}}nonexistent.cpp'
[clang-diagnostic-error]
@@ -31,6 +33,7 @@
// CHECK3: error: unknown argument: '-fan-unknown-option'
[clang-diagnostic-error]
// CHECK5: error: unknown argument: '-fan-option-from-compilation-database'
[clang-diagnostic-error]
+// CHECK7: :[[@LINE+4]]:9: warning: implicit conversion from 'double' to 'int'
changes value from 1.5 to 1 [clang-diagnostic-literal-conversion]
// CHECK2: :[[@LINE+3]]:9: warning: implicit conversion from 'double' to 'int'
changes value from 1.5 to 1 [clang-diagnostic-literal-conversion]
// CHECK3: :[[@LINE+2]]:9: warning: implicit conversion from 'double' to 'int'
changes value
// CHECK5: :[[@LINE+1]]:9: warning: implicit conversion from 'double' to 'int'
changes value
@@ -43,6 +46,7 @@
#define MACRO_FROM_COMMAND_LINE
// CHECK4: :[[@LINE-1]]:9: warning: 'MACRO_FROM_COMMAND_LINE' macro redefined
+// CHECK7-NOT: :[[@LINE-2]]:9: warning: 'MACRO_FROM_COMMAND_LINE' macro
redefined
#ifdef COMPILATION_ERROR
void f(int a) {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -111,6 +111,9 @@
be promoted to errors. For custom error promotion, use `-Werror=`
on the compiler command-line, irrespective of `Checks` (`--checks=`)
settings.
+- Fixed an issue where compiler warnings couldn't be suppressed using
+ `-Wno-` under C++20 and above.
+
New checks
^^
Index: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
===
--- clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
+++ clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
@@ -79,6 +79,9 @@
OverlayFS->pushOverlay(InMemoryFs);
Diags.setSourceManager();
+ // FIXME: Investigate whatever is there better way to initialize DiagEngine
+ // or whatever DiagEngine can be shared by multiple preprocessors
+ ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts());
LangOpts.Modules = false;
Index: clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
@@ -23,6 +23,8 @@
// RUN: clang-tidy -checks='-*,modernize-use-override,clang-diagnostic-macro-redefined' %T/diagnostics/input.cpp -- -DMACRO_FROM_COMMAND_LINE 2>&1 | FileCheck -check-prefix=CHECK4 -implicit-check-not='{{warning:|error:}}' %s
// RUN: not clang-tidy -checks='-*,clang-diagnostic-*,google-explicit-constructor' %T/diagnostics/input.cpp 2>&1 | FileCheck -check-prefix=CHECK5 -implicit-check-not='{{warning:|error:}}' %s
// RUN: not clang-tidy -checks='-*,modernize-use-override'
[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders
PiotrZSL updated this revision to Diff 543601.
PiotrZSL marked 3 inline comments as done.
PiotrZSL added a comment.
Review comments fixes
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D156056/new/
https://reviews.llvm.org/D156056
Files:
clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
Index: clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
@@ -23,6 +23,8 @@
// RUN: clang-tidy
-checks='-*,modernize-use-override,clang-diagnostic-macro-redefined'
%T/diagnostics/input.cpp -- -DMACRO_FROM_COMMAND_LINE 2>&1 | FileCheck
-check-prefix=CHECK4 -implicit-check-not='{{warning:|error:}}' %s
// RUN: not clang-tidy
-checks='-*,clang-diagnostic-*,google-explicit-constructor'
%T/diagnostics/input.cpp 2>&1 | FileCheck -check-prefix=CHECK5
-implicit-check-not='{{warning:|error:}}' %s
// RUN: not clang-tidy -checks='-*,modernize-use-override'
%T/diagnostics/input.cpp -- -DCOMPILATION_ERROR 2>&1 | FileCheck
-check-prefix=CHECK6 -implicit-check-not='{{warning:|error:}}' %s
+// RUN: clang-tidy
-checks='-*,modernize-use-override,clang-diagnostic-macro-redefined' %s --
-DMACRO_FROM_COMMAND_LINE -std=c++20 | FileCheck -check-prefix=CHECK4
-implicit-check-not='{{warning:|error:}}' %s
+// RUN: clang-tidy
-checks='-*,modernize-use-override,clang-diagnostic-macro-redefined,clang-diagnostic-literal-conversion'
%s -- -DMACRO_FROM_COMMAND_LINE -std=c++20 -Wno-macro-redefined | FileCheck
--check-prefix=CHECK7 -implicit-check-not='{{warning:|error:}}' %s
// CHECK1: error: no input files [clang-diagnostic-error]
// CHECK1: error: no such file or directory: '{{.*}}nonexistent.cpp'
[clang-diagnostic-error]
@@ -31,6 +33,7 @@
// CHECK3: error: unknown argument: '-fan-unknown-option'
[clang-diagnostic-error]
// CHECK5: error: unknown argument: '-fan-option-from-compilation-database'
[clang-diagnostic-error]
+// CHECK7: :[[@LINE+4]]:9: warning: implicit conversion from 'double' to 'int'
changes value from 1.5 to 1 [clang-diagnostic-literal-conversion]
// CHECK2: :[[@LINE+3]]:9: warning: implicit conversion from 'double' to 'int'
changes value from 1.5 to 1 [clang-diagnostic-literal-conversion]
// CHECK3: :[[@LINE+2]]:9: warning: implicit conversion from 'double' to 'int'
changes value
// CHECK5: :[[@LINE+1]]:9: warning: implicit conversion from 'double' to 'int'
changes value
@@ -43,6 +46,7 @@
#define MACRO_FROM_COMMAND_LINE
// CHECK4: :[[@LINE-1]]:9: warning: 'MACRO_FROM_COMMAND_LINE' macro redefined
+// CHECK7-NOT: :[[@LINE-2]]:9: warning: 'MACRO_FROM_COMMAND_LINE' macro
redefined
#ifdef COMPILATION_ERROR
void f(int a) {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -111,6 +111,9 @@
be promoted to errors. For custom error promotion, use `-Werror=`
on the compiler command-line, irrespective of `Checks` (`--checks=`)
settings.
+- Fixed an issue where compiler warnings couldn't be suppressed using
+ `-Wno-` under C++20 and above.
+
New checks
^^
Index: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
===
--- clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
+++ clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
@@ -79,6 +79,9 @@
OverlayFS->pushOverlay(InMemoryFs);
Diags.setSourceManager();
+ // FIXME: Investigate whatever is there better way to initialize DiagEngine
+ // or whatever DiagEngine can be shared by multiple preprocessors
+ ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts());
LangOpts.Modules = false;
Index: clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
@@ -23,6 +23,8 @@
// RUN: clang-tidy -checks='-*,modernize-use-override,clang-diagnostic-macro-redefined' %T/diagnostics/input.cpp -- -DMACRO_FROM_COMMAND_LINE 2>&1 | FileCheck -check-prefix=CHECK4 -implicit-check-not='{{warning:|error:}}' %s
// RUN: not clang-tidy -checks='-*,clang-diagnostic-*,google-explicit-constructor' %T/diagnostics/input.cpp 2>&1 | FileCheck -check-prefix=CHECK5 -implicit-check-not='{{warning:|error:}}' %s
// RUN: not clang-tidy -checks='-*,modernize-use-override' %T/diagnostics/input.cpp -- -DCOMPILATION_ERROR 2>&1 | FileCheck -check-prefix=CHECK6 -implicit-check-not='{{warning:|error:}}' %s
+//
[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders
carlosgalvezp accepted this revision. carlosgalvezp added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:75 + Diags(new DiagnosticIDs, +new DiagnosticOptions(Compiler.getDiagnosticOpts()), new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())), PiotrZSL wrote: > carlosgalvezp wrote: > > PiotrZSL wrote: > > > carlosgalvezp wrote: > > > > PiotrZSL wrote: > > > > > carlosgalvezp wrote: > > > > > > PiotrZSL wrote: > > > > > > > carlosgalvezp wrote: > > > > > > > > When downloading your patch, this seems to not be needed to > > > > > > > > make the tests pass, should it be removed? > > > > > > > No idea, it seem reasonable. > > > > > > Do you mean it seems reasonable to keep it, or reasonable to remove > > > > > > it? > > > > > reasonable to keep it, we want both DiagEngines to have same settings > > > > Reason I ask is that it seems the majority of `DiagnosticOptions` are > > > > initialized with default ctor: > > > > > > > > ``` > > > > $ git grep -E " DiagnosticOptions\(\w" | wc -l > > > > 3 > > > > $ git grep -E " DiagnosticOptions\(\)" | wc -l > > > > 74 > > > > ``` > > > > > > > > > we want both DiagEngines to have same settings > > > > > > > > Do you know where "the other" `DiagEngine` is initialized? The one I > > > > can find is initialized without the compiler opts. > > > > > > > > https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidy.cpp#L544 > > > > > > > > Since the added code does not have any impact on the result of the > > > > test, I'd argue that either the test is insufficient or the added code > > > > is dead code that should be removed. > > > sure, maybe ProcessWarningOptions will override it anyway, I didnt check > > > more deeply. > > In that case, could you please revert the change to this line, since it's > > not needed? > Yes, but later today. Ok! I will approve the patch now so you don't need to wait for me to land it before branching tomorrow Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156056/new/ https://reviews.llvm.org/D156056 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders
PiotrZSL marked an inline comment as done. PiotrZSL added inline comments. Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:75 + Diags(new DiagnosticIDs, +new DiagnosticOptions(Compiler.getDiagnosticOpts()), new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())), carlosgalvezp wrote: > PiotrZSL wrote: > > carlosgalvezp wrote: > > > PiotrZSL wrote: > > > > carlosgalvezp wrote: > > > > > PiotrZSL wrote: > > > > > > carlosgalvezp wrote: > > > > > > > When downloading your patch, this seems to not be needed to make > > > > > > > the tests pass, should it be removed? > > > > > > No idea, it seem reasonable. > > > > > Do you mean it seems reasonable to keep it, or reasonable to remove > > > > > it? > > > > reasonable to keep it, we want both DiagEngines to have same settings > > > Reason I ask is that it seems the majority of `DiagnosticOptions` are > > > initialized with default ctor: > > > > > > ``` > > > $ git grep -E " DiagnosticOptions\(\w" | wc -l > > > 3 > > > $ git grep -E " DiagnosticOptions\(\)" | wc -l > > > 74 > > > ``` > > > > > > > we want both DiagEngines to have same settings > > > > > > Do you know where "the other" `DiagEngine` is initialized? The one I can > > > find is initialized without the compiler opts. > > > > > > https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidy.cpp#L544 > > > > > > Since the added code does not have any impact on the result of the test, > > > I'd argue that either the test is insufficient or the added code is dead > > > code that should be removed. > > sure, maybe ProcessWarningOptions will override it anyway, I didnt check > > more deeply. > In that case, could you please revert the change to this line, since it's not > needed? Yes, but later today. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156056/new/ https://reviews.llvm.org/D156056 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders
carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:75 + Diags(new DiagnosticIDs, +new DiagnosticOptions(Compiler.getDiagnosticOpts()), new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())), PiotrZSL wrote: > carlosgalvezp wrote: > > PiotrZSL wrote: > > > carlosgalvezp wrote: > > > > PiotrZSL wrote: > > > > > carlosgalvezp wrote: > > > > > > When downloading your patch, this seems to not be needed to make > > > > > > the tests pass, should it be removed? > > > > > No idea, it seem reasonable. > > > > Do you mean it seems reasonable to keep it, or reasonable to remove it? > > > reasonable to keep it, we want both DiagEngines to have same settings > > Reason I ask is that it seems the majority of `DiagnosticOptions` are > > initialized with default ctor: > > > > ``` > > $ git grep -E " DiagnosticOptions\(\w" | wc -l > > 3 > > $ git grep -E " DiagnosticOptions\(\)" | wc -l > > 74 > > ``` > > > > > we want both DiagEngines to have same settings > > > > Do you know where "the other" `DiagEngine` is initialized? The one I can > > find is initialized without the compiler opts. > > > > https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidy.cpp#L544 > > > > Since the added code does not have any impact on the result of the test, > > I'd argue that either the test is insufficient or the added code is dead > > code that should be removed. > sure, maybe ProcessWarningOptions will override it anyway, I didnt check more > deeply. In that case, could you please revert the change to this line, since it's not needed? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156056/new/ https://reviews.llvm.org/D156056 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders
PiotrZSL marked an inline comment as done. PiotrZSL added inline comments. Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:75 + Diags(new DiagnosticIDs, +new DiagnosticOptions(Compiler.getDiagnosticOpts()), new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())), carlosgalvezp wrote: > PiotrZSL wrote: > > carlosgalvezp wrote: > > > PiotrZSL wrote: > > > > carlosgalvezp wrote: > > > > > When downloading your patch, this seems to not be needed to make the > > > > > tests pass, should it be removed? > > > > No idea, it seem reasonable. > > > Do you mean it seems reasonable to keep it, or reasonable to remove it? > > reasonable to keep it, we want both DiagEngines to have same settings > Reason I ask is that it seems the majority of `DiagnosticOptions` are > initialized with default ctor: > > ``` > $ git grep -E " DiagnosticOptions\(\w" | wc -l > 3 > $ git grep -E " DiagnosticOptions\(\)" | wc -l > 74 > ``` > > > we want both DiagEngines to have same settings > > Do you know where "the other" `DiagEngine` is initialized? The one I can find > is initialized without the compiler opts. > > https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidy.cpp#L544 > > Since the added code does not have any impact on the result of the test, I'd > argue that either the test is insufficient or the added code is dead code > that should be removed. sure, maybe ProcessWarningOptions will override it anyway, I didnt check more deeply. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156056/new/ https://reviews.llvm.org/D156056 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders
carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:75 + Diags(new DiagnosticIDs, +new DiagnosticOptions(Compiler.getDiagnosticOpts()), new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())), PiotrZSL wrote: > carlosgalvezp wrote: > > PiotrZSL wrote: > > > carlosgalvezp wrote: > > > > When downloading your patch, this seems to not be needed to make the > > > > tests pass, should it be removed? > > > No idea, it seem reasonable. > > Do you mean it seems reasonable to keep it, or reasonable to remove it? > reasonable to keep it, we want both DiagEngines to have same settings Reason I ask is that it seems the majority of `DiagnosticOptions` are initialized with default ctor: ``` $ git grep -E " DiagnosticOptions\(\w" | wc -l 3 $ git grep -E " DiagnosticOptions\(\)" | wc -l 74 ``` > we want both DiagEngines to have same settings Do you know where "the other" `DiagEngine` is initialized? The one I can find is initialized without the compiler opts. https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clang-tidy/ClangTidy.cpp#L544 Since the added code does not have any impact on the result of the test, I'd argue that either the test is insufficient or the added code is dead code that should be removed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156056/new/ https://reviews.llvm.org/D156056 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders
PiotrZSL marked an inline comment as done. PiotrZSL added inline comments. Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:75 + Diags(new DiagnosticIDs, +new DiagnosticOptions(Compiler.getDiagnosticOpts()), new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())), carlosgalvezp wrote: > PiotrZSL wrote: > > carlosgalvezp wrote: > > > When downloading your patch, this seems to not be needed to make the > > > tests pass, should it be removed? > > No idea, it seem reasonable. > Do you mean it seems reasonable to keep it, or reasonable to remove it? reasonable to keep it, we want both DiagEngines to have same settings Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156056/new/ https://reviews.llvm.org/D156056 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders
carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:75 + Diags(new DiagnosticIDs, +new DiagnosticOptions(Compiler.getDiagnosticOpts()), new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())), PiotrZSL wrote: > carlosgalvezp wrote: > > When downloading your patch, this seems to not be needed to make the tests > > pass, should it be removed? > No idea, it seem reasonable. Do you mean it seems reasonable to keep it, or reasonable to remove it? Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:83 Diags.setSourceManager(); + ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts()); PiotrZSL wrote: > carlosgalvezp wrote: > > PiotrZSL wrote: > > > carlosgalvezp wrote: > > > > A bit unclear to me why we should add this line here, grepping for this > > > > function in the repo I only find hits in the `clang` folder. How come > > > > it's not needed in other places? > > > We create here new Preprocessor (line 96) and new DiagEngine (line 74), > > > when C++20/Modules are enabled this class is register as an second > > > Preprocessor and both are (+-) executed. > > > Unfortunately when we pass `-Wno-macro-redefined` it's pass only to > > > original DiagEngine, and we run into situation when warning is suppressed > > > by first DiagEngine, but not by second that is used by second > > > Preprocessor. > > > > > > Passing DiagnosticOptions alone to DiagEngine looks to be insufficient, > > > as it's does not apply settings, only calling this function apply them. > > > (somehow). > > > This is gray area for me. > > > > > > More about problem here: > > > https://discourse.llvm.org/t/rfc-expand-modular-headers-ppcallbacks-problem-in-c-20/71628 > > Thanks for the explanation! I'm not sure what the best way forward is. > > Would it make sense to add some `TODO` or `FIXME` comment to further > > investigate in the future if we want that line of code ? > Yes, FIXME could be added, but ExpandModularHeadersPPCallbacks got also other > issues, for example with TargetTriple propagation and __has_builtin, looks > like all those got same source, simply we shoudn't create separate > Preprocessor. Agreed, the whole thing looks fishy. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156056/new/ https://reviews.llvm.org/D156056 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders
PiotrZSL added inline comments. Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:75 + Diags(new DiagnosticIDs, +new DiagnosticOptions(Compiler.getDiagnosticOpts()), new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())), carlosgalvezp wrote: > When downloading your patch, this seems to not be needed to make the tests > pass, should it be removed? No idea, it seem reasonable. Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:83 Diags.setSourceManager(); + ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts()); carlosgalvezp wrote: > PiotrZSL wrote: > > carlosgalvezp wrote: > > > A bit unclear to me why we should add this line here, grepping for this > > > function in the repo I only find hits in the `clang` folder. How come > > > it's not needed in other places? > > We create here new Preprocessor (line 96) and new DiagEngine (line 74), > > when C++20/Modules are enabled this class is register as an second > > Preprocessor and both are (+-) executed. > > Unfortunately when we pass `-Wno-macro-redefined` it's pass only to > > original DiagEngine, and we run into situation when warning is suppressed > > by first DiagEngine, but not by second that is used by second Preprocessor. > > > > Passing DiagnosticOptions alone to DiagEngine looks to be insufficient, as > > it's does not apply settings, only calling this function apply them. > > (somehow). > > This is gray area for me. > > > > More about problem here: > > https://discourse.llvm.org/t/rfc-expand-modular-headers-ppcallbacks-problem-in-c-20/71628 > Thanks for the explanation! I'm not sure what the best way forward is. Would > it make sense to add some `TODO` or `FIXME` comment to further investigate in > the future if we want that line of code ? Yes, FIXME could be added, but ExpandModularHeadersPPCallbacks got also other issues, for example with TargetTriple propagation and __has_builtin, looks like all those got same source, simply we shoudn't create separate Preprocessor. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156056/new/ https://reviews.llvm.org/D156056 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders
carlosgalvezp added a comment. A thought came to mind - since we are doing workarounds anyway, would it be easier to ask people to simply add `-clang-diagnostic*` to the `Checks` in their config file? It's fair to assume they will get those warnings when compiling the code. I feel the more workarounds we add in the code the harder it will be to clean it up later :) Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:75 + Diags(new DiagnosticIDs, +new DiagnosticOptions(Compiler.getDiagnosticOpts()), new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())), When downloading your patch, this seems to not be needed to make the tests pass, should it be removed? Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:83 Diags.setSourceManager(); + ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts()); PiotrZSL wrote: > carlosgalvezp wrote: > > A bit unclear to me why we should add this line here, grepping for this > > function in the repo I only find hits in the `clang` folder. How come it's > > not needed in other places? > We create here new Preprocessor (line 96) and new DiagEngine (line 74), when > C++20/Modules are enabled this class is register as an second Preprocessor > and both are (+-) executed. > Unfortunately when we pass `-Wno-macro-redefined` it's pass only to original > DiagEngine, and we run into situation when warning is suppressed by first > DiagEngine, but not by second that is used by second Preprocessor. > > Passing DiagnosticOptions alone to DiagEngine looks to be insufficient, as > it's does not apply settings, only calling this function apply them. > (somehow). > This is gray area for me. > > More about problem here: > https://discourse.llvm.org/t/rfc-expand-modular-headers-ppcallbacks-problem-in-c-20/71628 Thanks for the explanation! I'm not sure what the best way forward is. Would it make sense to add some `TODO` or `FIXME` comment to further investigate in the future if we want that line of code ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156056/new/ https://reviews.llvm.org/D156056 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders
PiotrZSL added inline comments. Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:83 Diags.setSourceManager(); + ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts()); carlosgalvezp wrote: > A bit unclear to me why we should add this line here, grepping for this > function in the repo I only find hits in the `clang` folder. How come it's > not needed in other places? We create here new Preprocessor (line 96) and new DiagEngine (line 74), when C++20/Modules are enabled this class is register as an second Preprocessor and both are (+-) executed. Unfortunately when we pass `-Wno-macro-redefined` it's pass only to original DiagEngine, and we run into situation when warning is suppressed by first DiagEngine, but not by second that is used by second Preprocessor. Passing DiagnosticOptions alone to DiagEngine looks to be insufficient, as it's does not apply settings, only calling this function apply them. (somehow). This is gray area for me. More about problem here: https://discourse.llvm.org/t/rfc-expand-modular-headers-ppcallbacks-problem-in-c-20/71628 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156056/new/ https://reviews.llvm.org/D156056 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders
carlosgalvezp added inline comments. Comment at: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp:83 Diags.setSourceManager(); + ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts()); A bit unclear to me why we should add this line here, grepping for this function in the repo I only find hits in the `clang` folder. How come it's not needed in other places? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156056/new/ https://reviews.llvm.org/D156056 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D156056: [clang-tidy] Initialize DiagnosticEngine in ExpandModularHeaders
PiotrZSL created this revision.
Herald added subscribers: carlosgalvezp, kbarton, xazax.hun, nemanjai.
Herald added a reviewer: njames93.
Herald added a project: All.
PiotrZSL requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Fix issue preventing suppression of compiler warnings with
-Wno- under C++20 and above. Add call to
ProcessWarningOptions and propagate DiagnosticOpts more properly.
Fixes: #56709, #61969
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D156056
Files:
clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
Index: clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
+++ clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
@@ -23,6 +23,8 @@
// RUN: clang-tidy
-checks='-*,modernize-use-override,clang-diagnostic-macro-redefined'
%T/diagnostics/input.cpp -- -DMACRO_FROM_COMMAND_LINE 2>&1 | FileCheck
-check-prefix=CHECK4 -implicit-check-not='{{warning:|error:}}' %s
// RUN: not clang-tidy
-checks='-*,clang-diagnostic-*,google-explicit-constructor'
%T/diagnostics/input.cpp 2>&1 | FileCheck -check-prefix=CHECK5
-implicit-check-not='{{warning:|error:}}' %s
// RUN: not clang-tidy -checks='-*,modernize-use-override'
%T/diagnostics/input.cpp -- -DCOMPILATION_ERROR 2>&1 | FileCheck
-check-prefix=CHECK6 -implicit-check-not='{{warning:|error:}}' %s
+// RUN: clang-tidy
-checks='-*,modernize-use-override,clang-diagnostic-macro-redefined' %s --
-DMACRO_FROM_COMMAND_LINE -std=c++20 | FileCheck -check-prefix=CHECK4
-implicit-check-not='{{warning:|error:}}' %s
+// RUN: clang-tidy
-checks='-*,modernize-use-override,clang-diagnostic-macro-redefined,clang-diagnostic-literal-conversion'
%s -- -DMACRO_FROM_COMMAND_LINE -std=c++20 -Wno-macro-redefined | FileCheck
--check-prefix=CHECK7 -implicit-check-not='{{warning:|error:}}' %s
// CHECK1: error: no input files [clang-diagnostic-error]
// CHECK1: error: no such file or directory: '{{.*}}nonexistent.cpp'
[clang-diagnostic-error]
@@ -31,6 +33,7 @@
// CHECK3: error: unknown argument: '-fan-unknown-option'
[clang-diagnostic-error]
// CHECK5: error: unknown argument: '-fan-option-from-compilation-database'
[clang-diagnostic-error]
+// CHECK7: :[[@LINE+4]]:9: warning: implicit conversion from 'double' to 'int'
changes value from 1.5 to 1 [clang-diagnostic-literal-conversion]
// CHECK2: :[[@LINE+3]]:9: warning: implicit conversion from 'double' to 'int'
changes value from 1.5 to 1 [clang-diagnostic-literal-conversion]
// CHECK3: :[[@LINE+2]]:9: warning: implicit conversion from 'double' to 'int'
changes value
// CHECK5: :[[@LINE+1]]:9: warning: implicit conversion from 'double' to 'int'
changes value
@@ -43,6 +46,7 @@
#define MACRO_FROM_COMMAND_LINE
// CHECK4: :[[@LINE-1]]:9: warning: 'MACRO_FROM_COMMAND_LINE' macro redefined
+// CHECK7-NOT: :[[@LINE-2]]:9: warning: 'MACRO_FROM_COMMAND_LINE' macro
redefined
#ifdef COMPILATION_ERROR
void f(int a) {
Index: clang-tools-extra/docs/ReleaseNotes.rst
===
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -111,6 +111,9 @@
be promoted to errors. For custom error promotion, use `-Werror=`
on the compiler command-line, irrespective of `Checks` (`--checks=`)
settings.
+- Fixed an issue where compiler warnings couldn't be suppressed using
+ `-Wno-` under C++20 and above.
+
New checks
^^
Index: clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
===
--- clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
+++ clang-tools-extra/clang-tidy/ExpandModularHeadersPPCallbacks.cpp
@@ -71,7 +71,8 @@
InMemoryFs(new llvm::vfs::InMemoryFileSystem),
Sources(Compiler.getSourceManager()),
// Forward the new diagnostics to the original DiagnosticConsumer.
- Diags(new DiagnosticIDs, new DiagnosticOptions,
+ Diags(new DiagnosticIDs,
+new DiagnosticOptions(Compiler.getDiagnosticOpts()),
new ForwardingDiagnosticConsumer(Compiler.getDiagnosticClient())),
LangOpts(Compiler.getLangOpts()) {
// Add a FileSystem containing the extra files needed in place of modular
@@ -79,6 +80,7 @@
OverlayFS->pushOverlay(InMemoryFs);
Diags.setSourceManager();
+ ProcessWarningOptions(Diags, Compiler.getDiagnosticOpts());
LangOpts.Modules = false;
Index: clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/diagnostic.cpp
+++
