[clang] [Clang] Silently ignore unknown warnings in `--warning-suppression-mappings` (PR #124141)
jyknight wrote: Abandoning in favor of kadircet's #125722 and #125714. https://github.com/llvm/llvm-project/pull/124141 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Silently ignore unknown warnings in `--warning-suppression-mappings` (PR #124141)
https://github.com/jyknight closed https://github.com/llvm/llvm-project/pull/124141 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Silently ignore unknown warnings in `--warning-suppression-mappings` (PR #124141)
@@ -547,11 +547,9 @@ void
WarningsSpecialCaseList::processSections(DiagnosticsEngine &Diags) {
StringRef DiagGroup = SectionEntry->getKey();
if (Diags.getDiagnosticIDs()->getDiagnosticsInGroup(
WarningFlavor, DiagGroup, GroupDiags)) {
- StringRef Suggestion =
- DiagnosticIDs::getNearestOption(WarningFlavor, DiagGroup);
- Diags.Report(diag::warn_unknown_diag_option)
- << static_cast(WarningFlavor) << DiagGroup
- << !Suggestion.empty() << Suggestion;
+ // If a diagnostic group name is unknown, simply ignore the
kadircet wrote:
> I realize it was intentional to emit unknown-warning diagnostics, but I
> really think we should not.
i still feel a little bit uneasy about leaving the user in the dark, when
they're trying to use the suppression mapping and it silently doesn't work
(even if it's due to a configuration error). so i'd rather punt on silently
ignoring, until concerns like version skew is really biting us in practice.
Most of the projects that maintain their own toolchain already needs to put
effort into fiddling with warning flags as they update their toolchain hence I
don't think we're creating any new maintenance surface here.
> Why parse the suppression file in the driver? Even if we do check
> ReportDiags, isn't it just wasted work to read it at all?
sorry my comment was a little misleading. i agree that we shouldn't try parsing
this in driver, there's simply no need. but we should still respect
`ReportDiags` in the implementation.
> I'd be happy to have you do so! (Even if a bit less happy than going with the
> simpler proposal, here).
thanks! i'll put together a patch soon
https://github.com/llvm/llvm-project/pull/124141
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Silently ignore unknown warnings in `--warning-suppression-mappings` (PR #124141)
@@ -547,11 +547,9 @@ void
WarningsSpecialCaseList::processSections(DiagnosticsEngine &Diags) {
StringRef DiagGroup = SectionEntry->getKey();
if (Diags.getDiagnosticIDs()->getDiagnosticsInGroup(
WarningFlavor, DiagGroup, GroupDiags)) {
- StringRef Suggestion =
- DiagnosticIDs::getNearestOption(WarningFlavor, DiagGroup);
- Diags.Report(diag::warn_unknown_diag_option)
- << static_cast(WarningFlavor) << DiagGroup
- << !Suggestion.empty() << Suggestion;
+ // If a diagnostic group name is unknown, simply ignore the
jyknight wrote:
I realize it was intentional to emit unknown-warning diagnostics, but I really
think we should not. Yes, that could catch a typo in the suppressions file, but
you could also typo the filenames in the same way without notice, so I don't
find that terribly persuasive as a use-case. Because this is a suppression
file, the primary notice that you've made a mistake is that the diagnostic you
expected to suppress did not get suppressed.
On the other hand, not emitting unknown-warning diagnostic is particularly
useful here, as it allows the suppressions file for a given codebase to be
easily used across multiple compilers. It allows users to easily write a file
which suppresses some instance of a newly-introduced diagnostic, yet doesn't
complain when the compiler doesn't yet support the suppressed-diagnostic.
Notably, on the version which doesn't recognize the flag, generally the
suppression is not _necessary_, since it doesn't know how to emit the
diagnostic anyways.
And there's not the ability to guard with `#if __has_warning` like you can if
adding a suppression via editing the source file.
> make sure we do respect rest of the warning options?
But, yes, if we _do_ continue to emit an unknown-warning diagnostic, it needs
to honor -Werror and -Wno-unknown-warning-option.
> in addition to that, we should propagate ReportDiags into
> DiagnosticsEngine::setDiagSuppressionMapping and not emit warnings when it's
> set to false (to prevent double emitting in driver & cc1).
Why parse the suppression file in the driver? Even if we do check ReportDiags,
isn't it just wasted work to read it at all?
> I am happy to do these changes myself, as they're more involved then what's
> in this PR currently.
I'd be happy to have you do so! (Even if a bit less happy than going with the
simpler proposal, here).
https://github.com/llvm/llvm-project/pull/124141
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Silently ignore unknown warnings in `--warning-suppression-mappings` (PR #124141)
@@ -547,11 +547,9 @@ void
WarningsSpecialCaseList::processSections(DiagnosticsEngine &Diags) {
StringRef DiagGroup = SectionEntry->getKey();
if (Diags.getDiagnosticIDs()->getDiagnosticsInGroup(
WarningFlavor, DiagGroup, GroupDiags)) {
- StringRef Suggestion =
- DiagnosticIDs::getNearestOption(WarningFlavor, DiagGroup);
- Diags.Report(diag::warn_unknown_diag_option)
- << static_cast(WarningFlavor) << DiagGroup
- << !Suggestion.empty() << Suggestion;
+ // If a diagnostic group name is unknown, simply ignore the
kadircet wrote:
it's intentional to emit these warnings so can we keep that but rather make
sure we do respect rest of the warning options?
we can do that by moving
https://github.com/llvm/llvm-project/blob/main/clang/lib/Basic/Warnings.cpp#L76-L84
past processing of other warning flags.
in addition to that, we should propagate `ReportDiags` into
`DiagnosticsEngine::setDiagSuppressionMapping` and not emit warnings when it's
set to false (to prevent double emitting in driver & cc1).
this is more of a FR, but we should also use source-manager and attach
source-location to these warnings.
--
I am happy to do these changes myself, as they're more involved then what's in
this PR currently. PLMK if you'd prefer that.
https://github.com/llvm/llvm-project/pull/124141
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Silently ignore unknown warnings in `--warning-suppression-mappings` (PR #124141)
https://github.com/kadircet requested changes to this pull request. https://github.com/llvm/llvm-project/pull/124141 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Silently ignore unknown warnings in `--warning-suppression-mappings` (PR #124141)
https://github.com/jyknight created
https://github.com/llvm/llvm-project/pull/124141
This allows the same file to be used on multiple Clang versions, without
generating output spam.
Also disable parsing of the suppressions file in the Driver, where it's not
needed. This was previously causing the diagnostics to be emitted twice, as the
file was being parsed twice.
I'll also note that if we do ever wish to emit non-fatal diagnostics from
parsing this file, it'll need more: the code deleted here emitted warnings
which were not possible for a user to disable, since the suppression file is
parsed _before_ the diagnostic state has been setup.
>From 56faefda954130f1f67d160d6ccc98e50f22674c Mon Sep 17 00:00:00 2001
From: James Y Knight
Date: Thu, 23 Jan 2025 11:02:20 -0500
Subject: [PATCH] [Clang] Silently ignore unknown warnings in
`--warning-suppression-mappings`.
This allows the same file to be used on multiple Clang versions,
without generating output spam.
Also disable parsing of the suppressions file in the Driver, where
it's not needed. This was previously causing the diagnostics to be
emitted twice, as the file was being parsed twice.
I'll also note that if we do ever wish to emit non-fatal diagnostics
from parsing this file, it'll need more: the code deleted here emitted
warnings which were not possible for a user to disable, since the
suppression file is parsed _before_ the diagnostic state has been
setup.
---
clang/lib/Basic/Diagnostic.cpp | 8 +++-
clang/test/Misc/Inputs/suppression-mapping.txt | 4
clang/tools/driver/driver.cpp | 3 +++
clang/unittests/Basic/DiagnosticTest.cpp | 3 +--
4 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index ae71758bc81e03..55efd5ffafcece 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/clang/lib/Basic/Diagnostic.cpp
@@ -547,11 +547,9 @@ void
WarningsSpecialCaseList::processSections(DiagnosticsEngine &Diags) {
StringRef DiagGroup = SectionEntry->getKey();
if (Diags.getDiagnosticIDs()->getDiagnosticsInGroup(
WarningFlavor, DiagGroup, GroupDiags)) {
- StringRef Suggestion =
- DiagnosticIDs::getNearestOption(WarningFlavor, DiagGroup);
- Diags.Report(diag::warn_unknown_diag_option)
- << static_cast(WarningFlavor) << DiagGroup
- << !Suggestion.empty() << Suggestion;
+ // If a diagnostic group name is unknown, simply ignore the
+ // suppressions. This allows use of a single suppression file on multiple
+ // versions of clang.
continue;
}
for (diag::kind Diag : GroupDiags)
diff --git a/clang/test/Misc/Inputs/suppression-mapping.txt
b/clang/test/Misc/Inputs/suppression-mapping.txt
index abe4fde0c265d5..cea8c50daee1c5 100644
--- a/clang/test/Misc/Inputs/suppression-mapping.txt
+++ b/clang/test/Misc/Inputs/suppression-mapping.txt
@@ -11,3 +11,7 @@ src:*foo/*=emit
[format=2]
src:*
src:*foo/*=emit
+
+# A warning group that clang doesn't know about should be silently ignored.
+[barglegunk]
+src:*
diff --git a/clang/tools/driver/driver.cpp b/clang/tools/driver/driver.cpp
index 74923247b7ee16..c68367c177910c 100644
--- a/clang/tools/driver/driver.cpp
+++ b/clang/tools/driver/driver.cpp
@@ -319,6 +319,9 @@ int clang_main(int Argc, char **Argv, const
llvm::ToolContext &ToolContext) {
IntrusiveRefCntPtr DiagOpts =
CreateAndPopulateDiagOpts(Args);
+ // The _driver_ (vs cc1) diagnostics engine shouldn't bother to load a
suppression file.
+ DiagOpts->DiagnosticSuppressionMappingsFile = "";
+
TextDiagnosticPrinter *DiagClient
= new TextDiagnosticPrinter(llvm::errs(), &*DiagOpts);
FixupDiagPrefixExeName(DiagClient, ProgName);
diff --git a/clang/unittests/Basic/DiagnosticTest.cpp
b/clang/unittests/Basic/DiagnosticTest.cpp
index e03d9a464df7f9..6c431ed81c4919 100644
--- a/clang/unittests/Basic/DiagnosticTest.cpp
+++ b/clang/unittests/Basic/DiagnosticTest.cpp
@@ -253,8 +253,7 @@ TEST_F(SuppressionMappingTest, UnknownDiagName) {
FS->addFile("foo.txt", /*ModificationTime=*/{},
llvm::MemoryBuffer::getMemBuffer("[non-existing-warning]"));
clang::ProcessWarningOptions(Diags, Diags.getDiagnosticOptions(), *FS);
- EXPECT_THAT(diags(), ElementsAre(WithMessage(
- "unknown warning option 'non-existing-warning'")));
+ EXPECT_THAT(diags(), IsEmpty());
}
TEST_F(SuppressionMappingTest, SuppressesGroup) {
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Silently ignore unknown warnings in `--warning-suppression-mappings` (PR #124141)
https://github.com/jyknight updated
https://github.com/llvm/llvm-project/pull/124141
>From 56faefda954130f1f67d160d6ccc98e50f22674c Mon Sep 17 00:00:00 2001
From: James Y Knight
Date: Thu, 23 Jan 2025 11:02:20 -0500
Subject: [PATCH 1/2] [Clang] Silently ignore unknown warnings in
`--warning-suppression-mappings`.
This allows the same file to be used on multiple Clang versions,
without generating output spam.
Also disable parsing of the suppressions file in the Driver, where
it's not needed. This was previously causing the diagnostics to be
emitted twice, as the file was being parsed twice.
I'll also note that if we do ever wish to emit non-fatal diagnostics
from parsing this file, it'll need more: the code deleted here emitted
warnings which were not possible for a user to disable, since the
suppression file is parsed _before_ the diagnostic state has been
setup.
---
clang/lib/Basic/Diagnostic.cpp | 8 +++-
clang/test/Misc/Inputs/suppression-mapping.txt | 4
clang/tools/driver/driver.cpp | 3 +++
clang/unittests/Basic/DiagnosticTest.cpp | 3 +--
4 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index ae71758bc81e03..55efd5ffafcece 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/clang/lib/Basic/Diagnostic.cpp
@@ -547,11 +547,9 @@ void
WarningsSpecialCaseList::processSections(DiagnosticsEngine &Diags) {
StringRef DiagGroup = SectionEntry->getKey();
if (Diags.getDiagnosticIDs()->getDiagnosticsInGroup(
WarningFlavor, DiagGroup, GroupDiags)) {
- StringRef Suggestion =
- DiagnosticIDs::getNearestOption(WarningFlavor, DiagGroup);
- Diags.Report(diag::warn_unknown_diag_option)
- << static_cast(WarningFlavor) << DiagGroup
- << !Suggestion.empty() << Suggestion;
+ // If a diagnostic group name is unknown, simply ignore the
+ // suppressions. This allows use of a single suppression file on multiple
+ // versions of clang.
continue;
}
for (diag::kind Diag : GroupDiags)
diff --git a/clang/test/Misc/Inputs/suppression-mapping.txt
b/clang/test/Misc/Inputs/suppression-mapping.txt
index abe4fde0c265d5..cea8c50daee1c5 100644
--- a/clang/test/Misc/Inputs/suppression-mapping.txt
+++ b/clang/test/Misc/Inputs/suppression-mapping.txt
@@ -11,3 +11,7 @@ src:*foo/*=emit
[format=2]
src:*
src:*foo/*=emit
+
+# A warning group that clang doesn't know about should be silently ignored.
+[barglegunk]
+src:*
diff --git a/clang/tools/driver/driver.cpp b/clang/tools/driver/driver.cpp
index 74923247b7ee16..c68367c177910c 100644
--- a/clang/tools/driver/driver.cpp
+++ b/clang/tools/driver/driver.cpp
@@ -319,6 +319,9 @@ int clang_main(int Argc, char **Argv, const
llvm::ToolContext &ToolContext) {
IntrusiveRefCntPtr DiagOpts =
CreateAndPopulateDiagOpts(Args);
+ // The _driver_ (vs cc1) diagnostics engine shouldn't bother to load a
suppression file.
+ DiagOpts->DiagnosticSuppressionMappingsFile = "";
+
TextDiagnosticPrinter *DiagClient
= new TextDiagnosticPrinter(llvm::errs(), &*DiagOpts);
FixupDiagPrefixExeName(DiagClient, ProgName);
diff --git a/clang/unittests/Basic/DiagnosticTest.cpp
b/clang/unittests/Basic/DiagnosticTest.cpp
index e03d9a464df7f9..6c431ed81c4919 100644
--- a/clang/unittests/Basic/DiagnosticTest.cpp
+++ b/clang/unittests/Basic/DiagnosticTest.cpp
@@ -253,8 +253,7 @@ TEST_F(SuppressionMappingTest, UnknownDiagName) {
FS->addFile("foo.txt", /*ModificationTime=*/{},
llvm::MemoryBuffer::getMemBuffer("[non-existing-warning]"));
clang::ProcessWarningOptions(Diags, Diags.getDiagnosticOptions(), *FS);
- EXPECT_THAT(diags(), ElementsAre(WithMessage(
- "unknown warning option 'non-existing-warning'")));
+ EXPECT_THAT(diags(), IsEmpty());
}
TEST_F(SuppressionMappingTest, SuppressesGroup) {
>From c67cf0bb57fb850aab42d5a382fdc15aaff2967d Mon Sep 17 00:00:00 2001
From: James Y Knight
Date: Thu, 23 Jan 2025 11:16:41 -0500
Subject: [PATCH 2/2] clang-format.
---
clang/tools/driver/driver.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/clang/tools/driver/driver.cpp b/clang/tools/driver/driver.cpp
index c68367c177910c..5ef64a6a6ad991 100644
--- a/clang/tools/driver/driver.cpp
+++ b/clang/tools/driver/driver.cpp
@@ -319,7 +319,8 @@ int clang_main(int Argc, char **Argv, const
llvm::ToolContext &ToolContext) {
IntrusiveRefCntPtr DiagOpts =
CreateAndPopulateDiagOpts(Args);
- // The _driver_ (vs cc1) diagnostics engine shouldn't bother to load a
suppression file.
+ // The _driver_ (vs cc1) diagnostics engine shouldn't bother to load a
+ // suppression file.
DiagOpts->DiagnosticSuppressionMappingsFile = "";
TextDiagnosticPrinter *DiagClient
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe
[clang] [Clang] Silently ignore unknown warnings in `--warning-suppression-mappings` (PR #124141)
github-actions[bot] wrote:
:warning: C/C++ code formatter, clang-format found issues in your code.
:warning:
You can test this locally with the following command:
``bash
git-clang-format --diff 517334bdb83deaae3be6fbc4fa5f1d721b01c0f0
56faefda954130f1f67d160d6ccc98e50f22674c --extensions cpp --
clang/lib/Basic/Diagnostic.cpp clang/tools/driver/driver.cpp
clang/unittests/Basic/DiagnosticTest.cpp
``
View the diff from clang-format here.
``diff
diff --git a/clang/tools/driver/driver.cpp b/clang/tools/driver/driver.cpp
index c68367c177..5ef64a6a6a 100644
--- a/clang/tools/driver/driver.cpp
+++ b/clang/tools/driver/driver.cpp
@@ -319,7 +319,8 @@ int clang_main(int Argc, char **Argv, const
llvm::ToolContext &ToolContext) {
IntrusiveRefCntPtr DiagOpts =
CreateAndPopulateDiagOpts(Args);
- // The _driver_ (vs cc1) diagnostics engine shouldn't bother to load a
suppression file.
+ // The _driver_ (vs cc1) diagnostics engine shouldn't bother to load a
+ // suppression file.
DiagOpts->DiagnosticSuppressionMappingsFile = "";
TextDiagnosticPrinter *DiagClient
``
https://github.com/llvm/llvm-project/pull/124141
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[clang] [Clang] Silently ignore unknown warnings in `--warning-suppression-mappings` (PR #124141)
llvmbot wrote:
@llvm/pr-subscribers-clang
Author: James Y Knight (jyknight)
Changes
This allows the same file to be used on multiple Clang versions, without
generating output spam.
Also disable parsing of the suppressions file in the Driver, where it's not
needed. This was previously causing the diagnostics to be emitted twice, as the
file was being parsed twice.
I'll also note that if we do ever wish to emit non-fatal diagnostics from
parsing this file, it'll need more: the code deleted here emitted warnings
which were not possible for a user to disable, since the suppression file is
parsed _before_ the diagnostic state has been setup.
---
Full diff: https://github.com/llvm/llvm-project/pull/124141.diff
4 Files Affected:
- (modified) clang/lib/Basic/Diagnostic.cpp (+3-5)
- (modified) clang/test/Misc/Inputs/suppression-mapping.txt (+4)
- (modified) clang/tools/driver/driver.cpp (+3)
- (modified) clang/unittests/Basic/DiagnosticTest.cpp (+1-2)
``diff
diff --git a/clang/lib/Basic/Diagnostic.cpp b/clang/lib/Basic/Diagnostic.cpp
index ae71758bc81e03..55efd5ffafcece 100644
--- a/clang/lib/Basic/Diagnostic.cpp
+++ b/clang/lib/Basic/Diagnostic.cpp
@@ -547,11 +547,9 @@ void
WarningsSpecialCaseList::processSections(DiagnosticsEngine &Diags) {
StringRef DiagGroup = SectionEntry->getKey();
if (Diags.getDiagnosticIDs()->getDiagnosticsInGroup(
WarningFlavor, DiagGroup, GroupDiags)) {
- StringRef Suggestion =
- DiagnosticIDs::getNearestOption(WarningFlavor, DiagGroup);
- Diags.Report(diag::warn_unknown_diag_option)
- << static_cast(WarningFlavor) << DiagGroup
- << !Suggestion.empty() << Suggestion;
+ // If a diagnostic group name is unknown, simply ignore the
+ // suppressions. This allows use of a single suppression file on multiple
+ // versions of clang.
continue;
}
for (diag::kind Diag : GroupDiags)
diff --git a/clang/test/Misc/Inputs/suppression-mapping.txt
b/clang/test/Misc/Inputs/suppression-mapping.txt
index abe4fde0c265d5..cea8c50daee1c5 100644
--- a/clang/test/Misc/Inputs/suppression-mapping.txt
+++ b/clang/test/Misc/Inputs/suppression-mapping.txt
@@ -11,3 +11,7 @@ src:*foo/*=emit
[format=2]
src:*
src:*foo/*=emit
+
+# A warning group that clang doesn't know about should be silently ignored.
+[barglegunk]
+src:*
diff --git a/clang/tools/driver/driver.cpp b/clang/tools/driver/driver.cpp
index 74923247b7ee16..c68367c177910c 100644
--- a/clang/tools/driver/driver.cpp
+++ b/clang/tools/driver/driver.cpp
@@ -319,6 +319,9 @@ int clang_main(int Argc, char **Argv, const
llvm::ToolContext &ToolContext) {
IntrusiveRefCntPtr DiagOpts =
CreateAndPopulateDiagOpts(Args);
+ // The _driver_ (vs cc1) diagnostics engine shouldn't bother to load a
suppression file.
+ DiagOpts->DiagnosticSuppressionMappingsFile = "";
+
TextDiagnosticPrinter *DiagClient
= new TextDiagnosticPrinter(llvm::errs(), &*DiagOpts);
FixupDiagPrefixExeName(DiagClient, ProgName);
diff --git a/clang/unittests/Basic/DiagnosticTest.cpp
b/clang/unittests/Basic/DiagnosticTest.cpp
index e03d9a464df7f9..6c431ed81c4919 100644
--- a/clang/unittests/Basic/DiagnosticTest.cpp
+++ b/clang/unittests/Basic/DiagnosticTest.cpp
@@ -253,8 +253,7 @@ TEST_F(SuppressionMappingTest, UnknownDiagName) {
FS->addFile("foo.txt", /*ModificationTime=*/{},
llvm::MemoryBuffer::getMemBuffer("[non-existing-warning]"));
clang::ProcessWarningOptions(Diags, Diags.getDiagnosticOptions(), *FS);
- EXPECT_THAT(diags(), ElementsAre(WithMessage(
- "unknown warning option 'non-existing-warning'")));
+ EXPECT_THAT(diags(), IsEmpty());
}
TEST_F(SuppressionMappingTest, SuppressesGroup) {
``
https://github.com/llvm/llvm-project/pull/124141
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
