[PATCH] D121175: [clang] Add -Wstart-no-unknown-warning-option/-Wend-no-unknown-warning-option.

[Daniel Thornburgh via Phabricator via cfe-commits]

mysterymath abandoned this revision.
mysterymath added a comment.

Given that we didn't reach a consensus on this one, I'll abandon this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121175

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

[PATCH] D121175: [clang] Add -Wstart-no-unknown-warning-option/-Wend-no-unknown-warning-option.

[Martin Storsjö via Phabricator via cfe-commits]

mstorsjo added a comment.

In D121175#3486104 , @phosek wrote:

> In D121175#3481547 , @MaskRay wrote:
>
>> Probably no from my view, as I got another internal feedback that "this 
>> seems rather crufty".
>> It seems that it is the build system maintainer's responsibility. If you add 
>> `-Wno-unknown-warning-option` temporarily, after a new clang is rolled, you 
>> may remove `-Wno-unknown-warning-option`.
>
> You can make the same exact argument about  
> `--start-no-unused-arguments`/`--end-no-unused-arguments` introduced in 
> D116503  which hasn't received the same 
> pushback.
>
> In D121175#3485891 , @dblaikie 
> wrote:
>
>> Would an explicit naming be more suitable than a region start/end? (I'd have 
>> considered this feedback for D116503  too, 
>> but didn't catch that one in review) The region based thing makes 
>> non-positional arguments weirdly positional (not that these are the first 
>> instances of that in compiler/linker tools, I don't think) - and I guess the 
>> override behavior is already positional to some degree.
>>
>> But something like -Wno-unused-command-line-argument=-Wfoobarbaz -Wfoobarbaz 
>> - sort of like password entry, type it twice so you've got less chance of 
>> mistakes?
>
> That would be fine with me as long as this also covers warnings included as 
> part of `-Wall` and `-Wextra` (that is you could do `-Wall 
> -Wno-unused-command-line-argument=-Wfoobarbaz`).
>
> I'd prefer if `--start-no-unused-arguments`/`--end-no-unused-arguments` and 
> `-Wstart-no-unknown-warning-option`/`-Wend-no-unknown-warning-option` were 
> symmetric but I'm not sure if we can change 
> `--start-no-unused-arguments`/`--end-no-unused-arguments` at this point since 
> it shipped in Clang 14.

Regarding usability - at least for `--start-no-unused-arguments`, in most cases 
I have 5-11 arguments within that start/stop region, so keeping it as a region 
is a fair bit more convenient than naming them one at a time.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121175

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

[PATCH] D121175: [clang] Add -Wstart-no-unknown-warning-option/-Wend-no-unknown-warning-option.

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

In D121175#3481547 , @MaskRay wrote:

> Probably no from my view, as I got another internal feedback that "this seems 
> rather crufty".
> It seems that it is the build system maintainer's responsibility. If you add 
> `-Wno-unknown-warning-option` temporarily, after a new clang is rolled, you 
> may remove `-Wno-unknown-warning-option`.

You can make the same exact argument about  
`--start-no-unused-arguments`/`--end-no-unused-arguments` introduced in D116503 
 which hasn't received the same pushback.

In D121175#3485891 , @dblaikie wrote:

> Would an explicit naming be more suitable than a region start/end? (I'd have 
> considered this feedback for D116503  too, 
> but didn't catch that one in review) The region based thing makes 
> non-positional arguments weirdly positional (not that these are the first 
> instances of that in compiler/linker tools, I don't think) - and I guess the 
> override behavior is already positional to some degree.
>
> But something like -Wno-unused-command-line-argument=-Wfoobarbaz -Wfoobarbaz 
> - sort of like password entry, type it twice so you've got less chance of 
> mistakes?

That would be fine with me as long as this also covers warnings included as 
part of `-Wall` and `-Wextra` (that is you could do `-Wall 
-Wno-unused-command-line-argument=-Wfoobarbaz`).

I'd prefer if `--start-no-unused-arguments`/`--end-no-unused-arguments` and 
`-Wstart-no-unknown-warning-option`/`-Wend-no-unknown-warning-option` were 
symmetric but I'm not sure if we can change 
`--start-no-unused-arguments`/`--end-no-unused-arguments` at this point since 
it shipped in Clang 14.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121175

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

[PATCH] D121175: [clang] Add -Wstart-no-unknown-warning-option/-Wend-no-unknown-warning-option.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Would an explicit naming be more suitable than a region start/end? (I'd have 
considered this feedback for D116503  too, 
but didn't catch that one in review) The region based thing makes 
non-positional arguments weirdly positional (not that these are the first 
instances of that in compiler/linker tools, I don't think) - and I guess the 
override behavior is already positional to some degree.

But something like -Wno-unused-command-line-argument=-Wfoobarbaz -Wfoobarbaz - 
sort of like password entry, type it twice so you've got less chance of 
mistakes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121175

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

[PATCH] D121175: [clang] Add -Wstart-no-unknown-warning-option/-Wend-no-unknown-warning-option.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Probably no from my view, as I got another internal feedback that "this seems 
rather crufty".
It seems that it is the build system maintainer's responsibility. If you add 
`-Wno-unknown-warning-option` temporarily, after a new clang is rolled, you may 
remove `-Wno-unknown-warning-option`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121175

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

[PATCH] D121175: [clang] Add -Wstart-no-unknown-warning-option/-Wend-no-unknown-warning-option.

[Daniel Thornburgh via Phabricator via cfe-commits]

mysterymath added a comment.
Herald added a subscriber: StephenFan.

Wanted to circle back around to this one, since this feature would still really 
help improve the workflow for ignoring warnings on build systems without deep 
compiler examination capabilities.

Can we move forward with this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121175

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

[PATCH] D121175: [clang] Add -Wstart-no-unknown-warning-option/-Wend-no-unknown-warning-option.

[Petr Hosek via Phabricator via cfe-commits]

phosek added a comment.

In D121175#3372056 , @condy wrote:

> IMHO, it's the duty of build systems. CMake provides 
> `check_cxx_compiler_flag` to report unknown options.

That's only true for build systems that perform configure time checks like 
CMake, but not for build systems like GN or Bazel.

This is motivated by our use case. We use GN and build with `-Werror`. We 
continuously test against tip-of-tree Clang to catch and report issues early. 
That becomes a problem when new warnings are introduced that trigger errors in 
our codebase. Since these can take anywhere from few days to a few weeks to 
address, we prefer suppressing the warning first while we do the cleanup.

We cannot use `-Wno-newly-introduced-warning` because our platform Clang 
doesn't yet recognize that warning, so instead we have to use 
`-Wno-unknown-warning-option`, but that may let other invalid warning options 
into our codebase (for example, misspelled warning options).

With this change, we can use `-Wstart-no-unknown-warning-option 
-Wno-newly-introduced-warning -Wend-no-unknown-warning-option` to only suppress 
the newly introduced warning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121175

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

[PATCH] D121175: [clang] Add -Wstart-no-unknown-warning-option/-Wend-no-unknown-warning-option.

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

I am also unsure about the usefulness.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121175

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

[PATCH] D121175: [clang] Add -Wstart-no-unknown-warning-option/-Wend-no-unknown-warning-option.

[Zhiwei Chen via Phabricator via cfe-commits]

condy added a comment.

IMHO, it's the duty of build systems. CMake provides `check_cxx_compiler_flag` 
to report unknown options.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121175

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

[PATCH] D121175: [clang] Add -Wstart-no-unknown-warning-option/-Wend-no-unknown-warning-option.

[Daniel Thornburgh via Phabricator via cfe-commits]

mysterymath created this revision.
mysterymath added reviewers: phosek, MaskRay.
Herald added a subscriber: dexonsmith.
Herald added a project: All.
mysterymath requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

These options allow guarding a region of warning options to allow
specify unknown options, akin to https://reviews.llvm.org/D116503.  This
allows supressing new warnings in a way that allows compilation with
older versions of clang, without globally setting
-Wno-unknown-warning-option.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D121175

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/lib/Basic/Warnings.cpp
  clang/test/Frontend/warning-options.cpp


Index: clang/test/Frontend/warning-options.cpp
===
--- clang/test/Frontend/warning-options.cpp
+++ clang/test/Frontend/warning-options.cpp
@@ -1,8 +1,11 @@
 // RUN: %clang_cc1 -Wmonkey -Wno-monkey -Wno-unused-command-line-arguments \
-// RUN:-Wno-unused-command-line-argument -Wmodule-build -Werror-vla 
-Rmodule-built %s 2>&1 | FileCheck %s
+// RUN:-Wno-unused-command-line-argument -Wmodule-build -Werror-vla 
-Rmodule-built \
+// RUN:-Wstart-no-unknown-warning-option -Wfoobarbaz 
-Wend-no-unknown-warning-option -Wplughxyzzy \
+// RUN:%s 2>&1 | FileCheck %s
 // CHECK: unknown warning option '-Wmonkey'
 // CHECK: unknown warning option '-Wno-monkey'
 // CHECK: unknown warning option '-Wno-unused-command-line-arguments'; did you 
mean '-Wno-unused-command-line-argument'?
 // CHECK: unknown warning option '-Wmodule-build'; did you mean 
'-Wmodule-conflict'?
 // CHECK-NEXT: unknown -Werror warning specifier: '-Werror-vla'
+// CHECK: unknown warning option '-Wplughxyzzy'
 // CHECK: unknown remark option '-Rmodule-built'; did you mean 
'-Rmodule-build'?
Index: clang/lib/Basic/Warnings.cpp
===
--- clang/lib/Basic/Warnings.cpp
+++ clang/lib/Basic/Warnings.cpp
@@ -189,6 +189,20 @@
 continue;
   }
 
+  if (Opt == "start-no-unknown-warning-option" ||
+  Opt == "end-no-unknown-warning-option") {
+// These flags should explicitly take effect during the report phase,
+// not the set-diagnostic phase, since they guard the processing of
+// specific arguments.
+if (Report) {
+  Diags.setSeverityForGroup(Flavor, "unknown-warning-option",
+Opt == "start-no-unknown-warning-option"
+? diag::Severity::Ignored
+: diag::Severity::Warning);
+}
+continue;
+  }
+
   if (Report) {
 if (DiagIDs->getDiagnosticsInGroup(Flavor, Opt, _Diags))
   EmitUnknownDiagWarning(Diags, Flavor, isPositive ? "-W" : "-Wno-",
Index: clang/docs/ClangCommandLineReference.rst
===
--- clang/docs/ClangCommandLineReference.rst
+++ clang/docs/ClangCommandLineReference.rst
@@ -1367,6 +1367,10 @@
 
 .. option:: -Wdeprecated, -Wno-deprecated
 
+Within the given flag region, prevent warning about unknown warning 
enable/disable flags.
+
+.. option:: -Wstart-no-unknown-warning-option, -Wend-no-unknown-warning-option
+
 Enable warnings for deprecated constructs and define \_\_DEPRECATED
 
 .. option:: -Wframe-larger-than=, -Wframe-larger-than


Index: clang/test/Frontend/warning-options.cpp
===
--- clang/test/Frontend/warning-options.cpp
+++ clang/test/Frontend/warning-options.cpp
@@ -1,8 +1,11 @@
 // RUN: %clang_cc1 -Wmonkey -Wno-monkey -Wno-unused-command-line-arguments \
-// RUN:-Wno-unused-command-line-argument -Wmodule-build -Werror-vla -Rmodule-built %s 2>&1 | FileCheck %s
+// RUN:-Wno-unused-command-line-argument -Wmodule-build -Werror-vla -Rmodule-built \
+// RUN:-Wstart-no-unknown-warning-option -Wfoobarbaz -Wend-no-unknown-warning-option -Wplughxyzzy \
+// RUN:%s 2>&1 | FileCheck %s
 // CHECK: unknown warning option '-Wmonkey'
 // CHECK: unknown warning option '-Wno-monkey'
 // CHECK: unknown warning option '-Wno-unused-command-line-arguments'; did you mean '-Wno-unused-command-line-argument'?
 // CHECK: unknown warning option '-Wmodule-build'; did you mean '-Wmodule-conflict'?
 // CHECK-NEXT: unknown -Werror warning specifier: '-Werror-vla'
+// CHECK: unknown warning option '-Wplughxyzzy'
 // CHECK: unknown remark option '-Rmodule-built'; did you mean '-Rmodule-build'?
Index: clang/lib/Basic/Warnings.cpp
===
--- clang/lib/Basic/Warnings.cpp
+++ clang/lib/Basic/Warnings.cpp
@@ -189,6 +189,20 @@
 continue;
   }
 
+  if (Opt == "start-no-unknown-warning-option" ||
+  Opt == "end-no-unknown-warning-option") {
+