Bug 188996 – Add IGNORE_WARNING_.* macros

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Guillaume Emont]

While working on #188598, I realized that we repeat a lot some boilerplate code to ignore warnings, and it is not always done in the safest way (to avoid creating other warnings on other compilers for instance). Typically, we see things like:

  #if COMPILER(CLANG) && defined(__has_warning)
  #pragma clang diagnostic push
  #if __has_warning("-Wfoo-bar")
  #pragma clang diagnostic ignored "-Wfoo-bar"
  #endif
  #endif

  // Code that triggers the warning.

  #if COMPILER(CLANG) && defined(__has_warning)
  #pragma clang diagnostic pop
  #endif

And we also see variants that don't check for the compiler type, or don't do the check with __has_warning(), or that ignore the warning on GCC instead of clang, or that ignores it on both compilers.

I propose to replace this boilerplate with macros that always do the checks that we want (compiler being used, and check __has_warning() if it's available).

[Guillaume Emont]

Created attachment 348180 [details]
Patch

This is a first proposal of the IGNORE_WARNING_.* macros.

[Guillaume Emont]

(In reply to Guillaume Emont from comment #1)
> Created attachment 348180 [details]
> Patch
> 
> This is a first proposal of the IGNORE_WARNING_.* macros.

Looks like my editor doesn't have the right settings of Objective C file, so my patch is not respecting the style guidelines yet it seems (tabs instead of spaces). I'll fix that in the next iteration of the patch, though I prefer to get it out already to be able to discuss it.

[Michael Catanzaro]

Comment on attachment 348180 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348180&action=review

Cool

(Read that in a John Oliver voice.)

BTW I still think this is worth notice on webkit-dev@.

> Source/WTF/wtf/Compiler.h:395
> +#define _COMPILER_CONCAT_I(a, b) a ## b
> +#define _COMPILER_CONCAT(a, b) _COMPILER_CONCAT_I(a, b)
> +
> +#define _COMPILER_STRINGIZE(exp) #exp

I would #undef them when you're done with them

> Source/WTF/wtf/Compiler.h:440
> +#define IGNORE_WARNING_CLANG(warning) do { IGNORE_WARNING_IMPL(clang, warning) } while (false)
> +#define IGNORE_WARNING_CLANG_END(warning) do { IGNORE_WARNING_END_IMPL(clang, warning) } while (false)
> +#define IGNORE_WARNING_CLANG_TOP_LEVEL(warning) IGNORE_WARNING_IMPL(clang, warning)
> +#define IGNORE_WARNING_CLANG_TOP_LEVEL_END(warning) IGNORE_WARNING_END_IMPL(clang, warning)

I kinda think getting rid of the TOP_LEVEL would be nice, to reduce the verbosity and complexity. The inner-function version with the do { } while is only different so that a semicolon can be used at the end, right? Generally the do { } while idiom is intended to make macros easier to use, not harder, and I think it is not really worth it in this case because now you've found up having to duplicate every macro here, and the user has to remember the differences between the two macros (one requires a trailing semicolon, the other doesn't).| It has kinda backfired here, IMO. So I would remove the do { } while versions and require that the semicolon is never used. I think it's not unnatural given that this macro is a replacement for pragmas, not a function, and  hopefully nobody is going to expect it can be used as a function.

[Michael Catanzaro]

Even so, it's an amazing patch: this is way cleaner than we had before.

[Guillaume Emont]

Comment on attachment 348180 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348180&action=review

>> Source/WTF/wtf/Compiler.h:395
>> +#define _COMPILER_STRINGIZE(exp) #exp
> 
> I would #undef them when you're done with them

I wish I could. But since they are used inside of macros, it looks like they need to be defined wherever the macro is used. I tried to alleviate this by using the _COMPILER_ prefix. I'm open to alternative ideas, but unfortunately, simply #undef'ing them is not an option.

>> Source/WTF/wtf/Compiler.h:440
>> +#define IGNORE_WARNING_CLANG_TOP_LEVEL_END(warning) IGNORE_WARNING_END_IMPL(clang, warning)
> 
> I kinda think getting rid of the TOP_LEVEL would be nice, to reduce the verbosity and complexity. The inner-function version with the do { } while is only different so that a semicolon can be used at the end, right? Generally the do { } while idiom is intended to make macros easier to use, not harder, and I think it is not really worth it in this case because now you've found up having to duplicate every macro here, and the user has to remember the differences between the two macros (one requires a trailing semicolon, the other doesn't).| It has kinda backfired here, IMO. So I would remove the do { } while versions and require that the semicolon is never used. I think it's not unnatural given that this macro is a replacement for pragmas, not a function, and  hopefully nobody is going to expect it can be used as a function.

I hear your argument, and I think it makes sense. Though I think that indeed this kind of question is worth asking on webkit-dev@.
To add to your proposition, if we were doing that, what about calling the macors "PRAGMA_IGNORE_WARNING.*" to make it clear that they are "like pragmas" and that's why we don't put semicolons afterwards?

[Michael Catanzaro]

(In reply to Guillaume Emont from comment #5)
> I wish I could. But since they are used inside of macros, it looks like they
> need to be defined wherever the macro is used. I tried to alleviate this by
> using the _COMPILER_ prefix. I'm open to alternative ideas, but
> unfortunately, simply #undef'ing them is not an option.

Heh! OK then, no big deal.

> I hear your argument, and I think it makes sense. Though I think that indeed
> this kind of question is worth asking on webkit-dev@.
> To add to your proposition, if we were doing that, what about calling the
> macors "PRAGMA_IGNORE_WARNING.*" to make it clear that they are "like
> pragmas" and that's why we don't put semicolons afterwards?

Sounds good to me. Maybe webkit-dev@ will have alternate naming suggestions.

[Guillaume Emont]

Created attachment 348296 [details]
Patch

New version of the patch rebased and fixing tabs in ObjC files

[EWS Watchlist]

Attachment 348296 [details] did not pass style-queue:


ERROR: Source/WebCore/xml/XPathGrammar.cpp:2157:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/xml/XPathGrammar.cpp:2160:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 196 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 348296 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348296&action=review

I like this.

> Source/WTF/wtf/Compiler.h:454
> +#define IGNORE_WARNING_GCC_OR_CLANG(warning) do { IGNORE_WARNING_IMPL(GCC, warning) } while (false)
> +#define IGNORE_WARNING_GCC_OR_CLANG_END(warning) do { IGNORE_WARNING_END_IMPL(GCC, warning) } while (false)
> +#define IGNORE_WARNING_GCC_OR_CLANG_TOP_LEVEL(warning) IGNORE_WARNING_IMPL(GCC, warning)
> +#define IGNORE_WARNING_GCC_OR_CLANG_TOP_LEVEL_END(warning) IGNORE_WARNING_END_IMPL(GCC, warning)

I think it would be much nicer if we just had two macros:
IGNORE_WARNING_BEGIN
IGNORE_WARNING_END

[Guillaume Emont]

Created attachment 348297 [details]
Patch

Fix compilation issue on Mac.

[EWS Watchlist]

Attachment 348297 [details] did not pass style-queue:


ERROR: Source/WebCore/xml/XPathGrammar.cpp:2157:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/xml/XPathGrammar.cpp:2160:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 196 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Guillaume Emont]

(In reply to Alex Christensen from comment #9)
> Comment on attachment 348296 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=348296&action=review
> 
> I like this.
> 
> > Source/WTF/wtf/Compiler.h:454
> > +#define IGNORE_WARNING_GCC_OR_CLANG(warning) do { IGNORE_WARNING_IMPL(GCC, warning) } while (false)
> > +#define IGNORE_WARNING_GCC_OR_CLANG_END(warning) do { IGNORE_WARNING_END_IMPL(GCC, warning) } while (false)
> > +#define IGNORE_WARNING_GCC_OR_CLANG_TOP_LEVEL(warning) IGNORE_WARNING_IMPL(GCC, warning)
> > +#define IGNORE_WARNING_GCC_OR_CLANG_TOP_LEVEL_END(warning) IGNORE_WARNING_END_IMPL(GCC, warning)
> 
> I think it would be much nicer if we just had two macros:
> IGNORE_WARNING_BEGIN
> IGNORE_WARNING_END
I'm not entirely sure this is the best, since the set of warnings are different between compilers, and we risk to add a warning on a given compiler if we try to ignore a warning that doesn't exist there. Currently this would work, since clang provides __has_warning(), and gcc silently ignores warning types it does not know, but I'm not sure this would work, if for instance we were to extend that to support visual studio.

[Michael Catanzaro]

(In reply to Guillaume Emont from comment #12)
> Currently
> this would work, since clang provides __has_warning(), and gcc silently
> ignores warning types it does not know, but I'm not sure this would work, if
> for instance we were to extend that to support visual studio.

GCC does not ignore warnings that it does not recognize:

[1/16] Compiling C object 'src/src@@ephymain@sha/ephy-action-bar.c.o'.
../../../../Projects/epiphany/src/ephy-action-bar.c:32:32: warning: unknown option after ‘#pragma GCC diagnostic’ kind [-Wpragmas]
 #pragma GCC diagnostic ignored "-Wasonteuhasoe"
                                ^~~~~~~~~~~~~~~~

So you need compiler guards somehow to avoid introducing new warnings (so it needs separate macros for GCC and Clang), and probably also compiler version guards for new warnings (which will just have to be done manually).

__has_warning() would be really nice to have in GCC. :(

[Alex Christensen]

I think the common case is that we will want to ignore a warning and we won't want to worry about whether it's the top level or not or if we only want to ignore it in one compiler for some reason.  Unrecognized pragmas are ignored by every compiler.

*_BEGIN and *_END macros are symmetric but * and *_END are not and will lead to forgotten ends.

[Michael Catanzaro]

(In reply to Alex Christensen from comment #14)
> I think the common case is that we will want to ignore a warning and we
> won't want to worry about whether it's the top level or not or if we only
> want to ignore it in one compiler for some reason.  Unrecognized pragmas are
> ignored by every compiler.

Again no, GCC warns about any use of #pragma clang, for instance. That's why we *always* guard pragmas with compiler guards.

[Michael Catanzaro]

E.g.

../../../../Projects/epiphany/src/ephy-action-bar.c:32: warning: ignoring #pragma clang diagnostic [-Wunknown-pragmas]
 #pragma clang diagnostic ignored "-Wformat"

I think Visual Studio warns too? I seem to remember that being a problem in the past. So if we have just a generic IGNORE_WARNING() it will always need to be wrapped in #if COMPILER() guards, both at the start and the end:

#if COMPILER(GCC_OR_CLANG)
IGNORE_WARNING("-Wformat")
#endif

printf(/* ... */);

#if COMPILER(GCC_OR_CLANG)
IGNORE_WARNING_END("-Wformat")
#endif

and that's only one line shorter than the current idiom:

#if COMPILER(GCC_OR_CLANG)
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wformat"
#endif

printf(/* ... */);

#if COMPILER(GCC_OR_CLANG)
#pragma GCC diagnostic pop
#endif

Hence having the compiler-specific macros seems useful, since that gets us down to:

IGNORE_GCC_OR_CLANG_WARNING("-Wformat")
printf(/* ... */);
END_IGNORE_GCC_OR_CLANG_WARNING("-Wformat")

which is very nice. We probably don't need the warning argument to the _END() macro either, since it's just doing a pop and must be ignored, but it seems nicer (more parallel) and more readable to keep it there.

Notice that I played a bit with the name of the macros in that last example (moving the END to the beginning rather than the end of the macro name).

[Michael Catanzaro]

Actually I suppose we could just build with -Wno-unknown-pragmas and -Wno-pragmas

Dunno if Visual Studio has similar options, though.

[Guillaume Emont]

(In reply to Michael Catanzaro from comment #16)
> E.g.
> 
> ../../../../Projects/epiphany/src/ephy-action-bar.c:32: warning: ignoring
> #pragma clang diagnostic [-Wunknown-pragmas]
>  #pragma clang diagnostic ignored "-Wformat"
> 
> I think Visual Studio warns too? I seem to remember that being a problem in
> the past. So if we have just a generic IGNORE_WARNING() it will always need
> to be wrapped in #if COMPILER() guards, both at the start and the end:
> 
> #if COMPILER(GCC_OR_CLANG)
> IGNORE_WARNING("-Wformat")
> #endif
> 
> printf(/* ... */);
> 
> #if COMPILER(GCC_OR_CLANG)
> IGNORE_WARNING_END("-Wformat")
> #endif
> 
> and that's only one line shorter than the current idiom:
> 
> #if COMPILER(GCC_OR_CLANG)
> #pragma GCC diagnostic push
> #pragma GCC diagnostic ignored "-Wformat"
> #endif
> 
> printf(/* ... */);
> 
> #if COMPILER(GCC_OR_CLANG)
> #pragma GCC diagnostic pop
> #endif
> 
> Hence having the compiler-specific macros seems useful, since that gets us
> down to:
> 
> IGNORE_GCC_OR_CLANG_WARNING("-Wformat")
> printf(/* ... */);
> END_IGNORE_GCC_OR_CLANG_WARNING("-Wformat")
> 
> which is very nice. We probably don't need the warning argument to the
> _END() macro either, since it's just doing a pop and must be ignored, but it
> seems nicer (more parallel) and more readable to keep it there.
We currently need the warning argument since the "diagnostic push" is guarded by an #if __has_warning() on clang. Alternatively, we could decide to agree to insert unnecessary pragma push/pop pairs to remove the argument, though I feel in some cases we would end up doing:

IGNORE_GCC_OR_CLANG_WARNING("-Wfoo-bar")

// hundreds of line of code

IGNORE_GCC_OR_CLANG_WARNING_END() // -Wfoo-bar

So for these two reasons (explicitness and avoiding unnecesary pragmas) I feel like it might be better to keep the argument for the _END macros.

> 
> Notice that I played a bit with the name of the macros in that last example
> (moving the END to the beginning rather than the end of the macro name).

That's a good idea. My name ordering showed that I'm not a native English speaker ;).

[Michael Catanzaro]

Speak of the devil, I just built with WebRTC enabled for the first time in a while and there are a bazillion warnings due to unguarded use of #pragma clang. I will be fixing that up shortly.

> So for these two reasons (explicitness and avoiding unnecesary pragmas) I
> feel like it might be better to keep the argument for the _END macros.

Yes.

> That's a good idea. My name ordering showed that I'm not a native English
> speaker ;).

Nah, your ordering was perfectly plausible too.

[Darin Adler]

Comment on attachment 348297 [details]
Patch

Seems like a reasonable idea. Better style, I suppose. Macros are ugly but these #pragma combinations are even more ugly in a way.

Seems excessive to pass in the same warning name to the "END" macros. Wordy, could get it wrong and nothing bad would happen. Please omit those extra arguments unless there is some good rationale I am missing.

Not sure we need the versions that allow semicolons. I think we could just use semicolon-free versions of these everywhere, not just the top level.

Since we are going to the trouble to visit all these call sites, I don’t think we should keep unnecessary distinctions of "clang-only", "clang or gcc", "gcc-only" in all these places. All of these should be the "both clang and gcc" versions unless there is some reason they can’t be.

It seems inelegant to pass in strings with the warnings to disable every time. We can do that for the more exotic warnings, but for common ones we should have specialized macros instead; better for clarity and also perhaps they can some day work as cross-compiler abstractions that work on other compilers. Here are the warnings that appear more than once:

 274 "-Wdeprecated-declarations"
  27 "-Wunknown-pragmas"
  24 "-Wunguarded-availability-new"
  13 "-Wunused-parameter"
  11 "-Wformat-nonliteral"
   9 "-Wreturn-type"
   6 "-Wnonnull"
   4 "-Wmissing-noreturn"
   4 "-Wimplicit-fallthrough"
   4 "-Wcast-qual"
   3 "-Wundefined-var-template"
   2 "-Wunused-private-field"
   2 "-Wtype-limits"
   2 "-Wnullability-completeness"
   2 "-Wformat-security"
   2 "-Wenum-compare"

I suggest we make warning-specific macros for at least the first one of these, and possibly a few more.

I’m not even sure that the word "WARNING" would need to appear in the all these macros. The ones about deprecated declarations could be:

    ALLOW_DEPRECATED_DECLARATIONS_START
    ALLOW_DEPRECATED_DECLARATIONS_END

And so on.

[Guillaume Emont]

Hi Darin,

Thanks for your thoughtful review, here are my answers to your comments, some of them bring more questions.

(In reply to Darin Adler from comment #20)
> Comment on attachment 348297 [details]
> Patch
> 
> Seems like a reasonable idea. Better style, I suppose. Macros are ugly but
> these #pragma combinations are even more ugly in a way.
> 
> Seems excessive to pass in the same warning name to the "END" macros. Wordy,
> could get it wrong and nothing bad would happen. Please omit those extra
> arguments unless there is some good rationale I am missing.
As I explained in https://bugs.webkit.org/show_bug.cgi?id=188996#c18 the rationale to keep the argument is:
 - consistency and clarity (we don't need to add a "// -Wfoo-bar" comment at the end when a large portion of code is within the ignore)
 - removing the argument to the _END macros would mean we have to always emit the diagnostic push/pop even if we don't add an ignore

If you believe that considering these two aspects it's still better to remove the argument from _END macros, I'm happy to do it.

> 
> Not sure we need the versions that allow semicolons. I think we could just
> use semicolon-free versions of these everywhere, not just the top level.

Ok, I will make the simplification.

> 
> Since we are going to the trouble to visit all these call sites, I don’t
> think we should keep unnecessary distinctions of "clang-only", "clang or
> gcc", "gcc-only" in all these places. All of these should be the "both clang
> and gcc" versions unless there is some reason they can’t be.

 - As Michael was pointing out, gcc warns if you try to ignore a warning it does not know about, so at least some of the clang only warnings need to remain that way.
 - We could change the gcc-only warnings to gcc or clang¸ since clang provides __has_warning(). Would you like me to do that?

> 
> It seems inelegant to pass in strings with the warnings to disable every
> time. We can do that for the more exotic warnings, but for common ones we
> should have specialized macros instead; better for clarity and also perhaps
> they can some day work as cross-compiler abstractions that work on other
> compilers. Here are the warnings that appear more than once:
> 
>  274 "-Wdeprecated-declarations"
>   27 "-Wunknown-pragmas"
>   24 "-Wunguarded-availability-new"
>   13 "-Wunused-parameter"
>   11 "-Wformat-nonliteral"
>    9 "-Wreturn-type"
>    6 "-Wnonnull"
>    4 "-Wmissing-noreturn"
>    4 "-Wimplicit-fallthrough"
>    4 "-Wcast-qual"
>    3 "-Wundefined-var-template"
>    2 "-Wunused-private-field"
>    2 "-Wtype-limits"
>    2 "-Wnullability-completeness"
>    2 "-Wformat-security"
>    2 "-Wenum-compare"
> 
> I suggest we make warning-specific macros for at least the first one of
> these, and possibly a few more.
> 
> I’m not even sure that the word "WARNING" would need to appear in the all
> these macros. The ones about deprecated declarations could be:
> 
>     ALLOW_DEPRECATED_DECLARATIONS_START
>     ALLOW_DEPRECATED_DECLARATIONS_END
> 
> And so on.

Makes sense, I'll work on that.

[Michael Catanzaro]

(In reply to Guillaume Emont from comment #21)
>  - As Michael was pointing out, gcc warns if you try to ignore a warning it
> does not know about, so at least some of the clang only warnings need to
> remain that way.

I think we can control this behavior using -Wno-pragmas and -Wno-unknown-pragmas:

       -Wunknown-pragmas
           Warn when a "#pragma" directive is encountered that is not
           understood by GCC.  If this command-line option is used, warnings
           are even issued for unknown pragmas in system header files.  This
           is not the case if the warnings are only enabled by the -Wall
           command-line option.

       -Wno-pragmas
           Do not warn about misuses of pragmas, such as incorrect parameters,
           invalid syntax, or conflicts between pragmas.  See also
           -Wunknown-pragmas.

I suggest we do use -Wno-unknown-pragmas, because it's annoying to see GCC complain every time #pragma clang is used. It's obvious that when #pragma clang is used, the pragma is intended to be ignored by GCC. It's not helpful for that to trigger warnings each time. If the GCC developers think -Wunknown-pragmas is useful, they should make it only warn when it sees unknown pragmas after #pragma GCC and not otherwise. (I'll remind developers that #pragma GCC should be used whenever a warning is not Clang-specific.)

The value of -Wno-pragmas is less-clear to me, but it would allow us to get the behavior requested by Darin and Alex to have just one macro not specific to each compiler, presuming Visual Studio does not have similar warnings or they can be turned off.

[Darin Adler]

(In reply to Guillaume Emont from comment #21)
> As I explained in https://bugs.webkit.org/show_bug.cgi?id=188996#c18 the
> rationale to keep the argument is:
>  - consistency and clarity (we don't need to add a "// -Wfoo-bar" comment at
> the end when a large portion of code is within the ignore)
>  - removing the argument to the _END macros would mean we have to always
> emit the diagnostic push/pop even if we don't add an ignore
> 
> If you believe that considering these two aspects it's still better to
> remove the argument from _END macros, I'm happy to do it.

I do. My view is that an ignored argument that can be wrong is not *better* than a comment. In fact it’s *worse* because it’s a comment in disguise and a comment that you always must include. You could put "<<<just making the macro happen>>>" in and it would work compile just fine.

I don’t think that END macros should have comments in any normal circumstances; they are going to be just a line or two down from the start macros. Just as we wouldn’t want to comment every close brace with an explanation of the open brace from a line or two above. Maybe occasionally a comment would be needed for clarity but it seems unlikely.

I’m not at all concerned about compiling more push/pop pairs but maybe you could show me an example of a specific case where that is bad and help me understand why you are concerned about it.

>  - As Michael was pointing out, gcc warns if you try to ignore a warning it
> does not know about, so at least some of the clang only warnings need to
> remain that way.

That’s fine, but I believe the default should be the "both" version and the unusual, longer-named, more-special version should be the single-compiler only versions.

The goal of the compiler file is to create mostly compiler-independent abstractions and one thing I don’t like about this is that we also support the Windows compiler and these macros don’t help us ignore the corresponding warnings on Windows in an elegant way. Not a problem in the short term, but a problem with the design. Unless none of these warning concepts make sense on Windows. But that seems unlikely.

There are some strange intersections here. The "ignore deprecation" warning is Apple-platform-specific so that explains why it will never come up on Windows, and that's more than half of the use of this.

>  - We could change the gcc-only warnings to gcc or clang¸ since clang
> provides __has_warning(). Would you like me to do that?

Generally speaking, yes I would, if there are a lot of these. But to be sure I’d need some examples of specifics.

Our default should be cross-compiler things, and our compiler-specific things should be treated as "grudging complexities". The issue with GCC complaining about unknown warnings is not just an issue for clang-specific things, by the way, it’s an issue for multiple versions of GCC. If a new version adds a new warning, we need to make the warning disabling apply only under the newer compiler.

I’m not 100% sure that we should do -Wno-unknown-pragmas, by the way, since that warning aspires to be a useful way to detect typos in pragmas and I would love to do that even if it creates a bit of inconvenience for us. But there may be no practical way to leave it on and support multiple compilers and compiler versions elegantly. I don’t know why clang doesn’t offer a similar warning.

[Michael Catanzaro]

(In reply to Darin Adler from comment #23) 
> There are some strange intersections here. The "ignore deprecation" warning
> is Apple-platform-specific so that explains why it will never come up on
> Windows, and that's more than half of the use of this.

FYI, we have -Wdeprecated-declarations on Linux too, and we do need to use it occasionally, usually in API tests for testing that we don't break our own deprecated APIs.

[Guillaume Emont]

(In reply to Darin Adler from comment #23)
> (In reply to Guillaume Emont from comment #21)
> > As I explained in https://bugs.webkit.org/show_bug.cgi?id=188996#c18 the
> > rationale to keep the argument is:
> >  - consistency and clarity (we don't need to add a "// -Wfoo-bar" comment at
> > the end when a large portion of code is within the ignore)
> >  - removing the argument to the _END macros would mean we have to always
> > emit the diagnostic push/pop even if we don't add an ignore
> > 
> > If you believe that considering these two aspects it's still better to
> > remove the argument from _END macros, I'm happy to do it.
> 
> I do. My view is that an ignored argument that can be wrong is not *better*
> than a comment. In fact it’s *worse* because it’s a comment in disguise and
> a comment that you always must include. You could put "<<<just making the
> macro happen>>>" in and it would work compile just fine.
> 
> I don’t think that END macros should have comments in any normal
> circumstances; they are going to be just a line or two down from the start
> macros. Just as we wouldn’t want to comment every close brace with an
> explanation of the open brace from a line or two above. Maybe occasionally a
> comment would be needed for clarity but it seems unlikely.
> 
> I’m not at all concerned about compiling more push/pop pairs but maybe you
> could show me an example of a specific case where that is bad and help me
> understand why you are concerned about it.

I can't think of any issue that extra push/pop pairs would cause, other than maybe a negligible increase in compilation time (though it could be a decrease as we would be simplifying the macro), so I think I'm just being overly cautious. I'll work on removing the argument to the _END() macros.
> 
> >  - As Michael was pointing out, gcc warns if you try to ignore a warning it
> > does not know about, so at least some of the clang only warnings need to
> > remain that way.
> 
> That’s fine, but I believe the default should be the "both" version and the
> unusual, longer-named, more-special version should be the single-compiler
> only versions.
> 
> The goal of the compiler file is to create mostly compiler-independent
> abstractions and one thing I don’t like about this is that we also support
> the Windows compiler and these macros don’t help us ignore the corresponding
> warnings on Windows in an elegant way. Not a problem in the short term, but
> a problem with the design. Unless none of these warning concepts make sense
> on Windows. But that seems unlikely.
> 
> There are some strange intersections here. The "ignore deprecation" warning
> is Apple-platform-specific so that explains why it will never come up on
> Windows, and that's more than half of the use of this.
> 
> >  - We could change the gcc-only warnings to gcc or clang¸ since clang
> > provides __has_warning(). Would you like me to do that?
> 
> Generally speaking, yes I would, if there are a lot of these. But to be sure
> I’d need some examples of specifics.
> 
> Our default should be cross-compiler things, and our compiler-specific
> things should be treated as "grudging complexities". The issue with GCC
> complaining about unknown warnings is not just an issue for clang-specific
> things, by the way, it’s an issue for multiple versions of GCC. If a new
> version adds a new warning, we need to make the warning disabling apply only
> under the newer compiler.
I feel like the case of a warning only existing in a subset of the gcc versions we support should be rare enough that we can leave with guarding the IGNORE_WARNING with a GCC_VERSION_AT_LEAST() in these cases.

All this considered, and to try to move forward, my proposal would be to have macros like this:

  IGNORE_WARNING_BEGIN("-Wfoo-bar")
  // some code
  IGNORE_WARNING_END

They would only expand to something on gcc or clang, and check __has_warning() when it is defined. I don't think it would make sense to try and expand them on MSVC as it expects a warning id number and not a string, so we don't have compatibility. And I will look at our current warning ignores and try to use this version wherever the warning exists on both compilers.

Then add similar pairs IGNORE_GCC_WARNING_BEGIN/END, IGNORE_CLANG_WARNING_BEGIN/END and IGNORE_MSVC_WARNING_BEGIN/END for warnings that are specific to these compilers.

Finally, as you suggested, add things like:
    ALLOW_DEPRECATED_DECLARATIONS_BEGIN
    ALLOW_DEPRECATED_DECLARATIONS_END
for the most common warnings. These could be implemented for MSVC as well, though I don't really know how to find the right msvc warning number for various situations, so I'd rather leave that part for future implementation by a windows developer who has access to msvc. I don't even know if that would be useful, since looking at the code, it seems that the msvc's "#pragma warning( disable: XXX)" are often in different places from the warning ignores of gcc/clang.

What do you think of that proposal?

> 
> I’m not 100% sure that we should do -Wno-unknown-pragmas, by the way, since
> that warning aspires to be a useful way to detect typos in pragmas and I
> would love to do that even if it creates a bit of inconvenience for us. But
> there may be no practical way to leave it on and support multiple compilers
> and compiler versions elegantly. I don’t know why clang doesn’t offer a
> similar warning.

I did some checking, and -Wno-unknown-pragmas does not change anything, you still get a warning if you do something like:
#pragma GCC diagnostic ignored "-Wclang-specific-warning"

We don't need to be worried about gcc warning about a "#pragma clang ..." since we can make sure the macros only emits the right kind of pragma for gcc when COMPILER(GCC) is defined.

[Darin Adler]

(In reply to Guillaume Emont from comment #25)
> These could be implemented for MSVC as well,
> though I don't really know how to find the right msvc warning number for
> various situations

Right, I am not suggesting that you preemptively implement these for MSVC. If a corresponding warning exists then we can do that later as needed.

> What do you think of that proposal?

Sounds fine.

[Guillaume Emont]

In looking at the most common ignored warnings, -Wunknown-pragmas is high on the list, though almost all of its uses were introduced in r220577. It's unclear to me whether this could be replaced by using a __has_warning() check or there is another reason to ignore -Wunknown-pragmas here.
Adding Dan Bernstein in Cc in the hope that he can enlighten me.

[Darin Adler]

I suggest you do a special purpose macro for the "-Wunguarded-availability-new" warning. You can use "-Wunknown-pragmas" in the implementation of it at first. Then remove it later if we figure out how to do it with __has_warning, but it won’t require visiting each call site.

(In reply to Guillaume Emont from comment #27)
> In looking at the most common ignored warnings, -Wunknown-pragmas is high on
> the list, though almost all of its uses were introduced in r220577. It's
> unclear to me whether this could be replaced by using a __has_warning()
> check or there is another reason to ignore -Wunknown-pragmas here.
> Adding Dan Bernstein in Cc in the hope that he can enlighten me.

I don’t know whether __has_warning() could be used instead, which tells you that I didn’t consider it at the time, simply because I didn’t think to do so. If it works for other warning-ignoring in our codebase I don’t see a reason why it wouldn’t work for unguarded-availability-new.

[Guillaume Emont]

(In reply to mitz from comment #29)
> (In reply to Guillaume Emont from comment #27)
> > In looking at the most common ignored warnings, -Wunknown-pragmas is high on
> > the list, though almost all of its uses were introduced in r220577. It's
> > unclear to me whether this could be replaced by using a __has_warning()
> > check or there is another reason to ignore -Wunknown-pragmas here.
> > Adding Dan Bernstein in Cc in the hope that he can enlighten me.
> 
> I don’t know whether __has_warning() could be used instead, which tells you
> that I didn’t consider it at the time, simply because I didn’t think to do
> so. If it works for other warning-ignoring in our codebase I don’t see a
> reason why it wouldn’t work for unguarded-availability-new.

Ok, thanks for the information!

I couldn't figure out how to check that easily for older versions, but __has_warning() is supported from at least clang 3.1:
http://releases.llvm.org/3.1/tools/clang/docs/LanguageExtensions.html#__has_warning

It's not clear to me what is the earliest version of clang we support, but I imagine it is later than that, meaning we can safely rely on __has_warning() and don't need -Wunkown-pragmas for that case.

[Darin Adler]

I don’t happen to know what is the oldest version of clang we need to support either, but I do know that __has_warning is already used in JavaScriptCore and WebCore:

Source/JavaScriptCore/API/JSCallbackObject.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructor.h
Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototype.h
Source/JavaScriptCore/runtime/Options.cpp
Source/WebKit/WebProcess/WebCoreSupport/WebAlternativeTextClient.h
Source/WebKitLegacy/mac/WebCoreSupport/WebAlternativeTextClient.h

So I’d say it’s safe to use it more without adverse effect on the WebKit project.

[Guillaume Emont]

Created attachment 348780 [details]
Patch

New version that should address all the review comments. Let's see if it compiles for all platforms on EWS ;). It created no problem compiling the WPE port for x86_64 with both gcc and clang in release and debug.

[EWS Watchlist]

Attachment 348780 [details] did not pass style-queue:


ERROR: Source/WebCore/xml/XPathGrammar.cpp:2157:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/xml/XPathGrammar.cpp:2160:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 196 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Guillaume Emont]

Created attachment 348808 [details]
Patch

Added a missing #import to fix mac compilation.

[EWS Watchlist]

Attachment 348808 [details] did not pass style-queue:


ERROR: Source/WebCore/xml/XPathGrammar.cpp:2157:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/xml/XPathGrammar.cpp:2160:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 196 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Guillaume Emont]

Ok, it seems to look like I'm not supposed to use WTF stuff in WebKitLegacy/mac/History/WebHistory.h, so I'm gonna revert my changes to it.

[Guillaume Emont]

Created attachment 348811 [details]
Patch

Don't try to use WTF stuff in WebKitLegacy/mac/History/WebHistory.h any more.

[EWS Watchlist]

Attachment 348811 [details] did not pass style-queue:


ERROR: Source/WebCore/xml/XPathGrammar.cpp:2157:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/xml/XPathGrammar.cpp:2160:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 195 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Guillaume Emont]

Created attachment 348812 [details]
Patch

This new version should fix mac-32. A deprecated call was previously ignored because of an erroneous diagnostic push instead of pop, I added ALLOW_DEPRECATED_DECLARATIONS_BEGIN/END around a GetWindowRegion() call in WebKitLegacy/mac/Carbon/HIWebView.mm

[EWS Watchlist]

Attachment 348812 [details] did not pass style-queue:


ERROR: Source/WebCore/xml/XPathGrammar.cpp:2157:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/xml/XPathGrammar.cpp:2160:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 195 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Guillaume Emont]

Ping reviewer

[Darin Adler]

Comment on attachment 348812 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348812&action=review

Patch looks like it was carefully done and I did not spot any mistakes.

I have some suggestions for tweaking the names of things.

> Source/JavaScriptCore/API/JSCallbackObject.h:161
> +    IGNORE_CLANG_WARNING_BEGIN("-Wundefined-var-template")

I think the macros should take arguments that are strings without the "-W" prefix, and the macros should add the "-W" prefix.

> Source/JavaScriptCore/runtime/ConfigFile.cpp:494
> +                IGNORE_GCC_WARNING_BEGIN("-Wstringop-truncation")

No need to use the "GCC" version here since it’s already inside #if COMPILER(GCC).

I personally prefer that we use the most exotic macro, then one with the longer name with the compiler check built in, only when it’s needed. Rather than always preferring it to help emphasize the fact that the warning is only for one compiler. We could choose either style.

> Source/WTF/wtf/Assertions.cpp:82
> -#pragma clang diagnostic push
> -#pragma clang diagnostic ignored "-Wdeprecated-declarations"
> +ALLOW_DEPRECATED_DECLARATIONS_BEGIN

This should be indented to match the code, rather than put at the far left to match where the #pragma was, to be consistent with the style you selected elsewhere. I think it’s a good style choice, and best to be consistent.

> Source/WTF/wtf/Compiler.h:419
> +#if !defined(IGNORE_GCC_WARNING_BEGIN) && COMPILER(GCC)

If seems that we have extra #if !defined guards. Does someone need to override these macros with definitions done before including this source file? We could add that feature (allowing an override) later if we need it; I am pretty sure we don’t need it and I would prefer to omit the guards except in cases where we use them to avoid complex #else constructions.

> Source/WTF/wtf/Compiler.h:446
> +#define IGNORE_WARNING_BEGIN(warning)
> +#define IGNORE_WARNING_END

I am not sure if "ignore warning" is better than "ignore warnings". After all, it ignores one specific type of warning, but any number of occurrences of the code that would warn. I think I slightly prefer the term "warnings" for these macros.

Also might be better terminology to use the term "suppress warnings" rather than "ignore warnings". Not totally sure about that one.

> Source/WTF/wtf/Compiler.h:456
> +#define ALLOW_NEW_API_BEGIN IGNORE_CLANG_WARNING_BEGIN("-Wunguarded-availability-new")
> +#define ALLOW_NEW_API_END IGNORE_CLANG_WARNING_END

I’m not sure the phrase "allow new API" is quite specific enough. Maybe someone more familiar with the exact semantics can suggest a better name. Maybe it should be ALLOW_NEW_API_WITHOUT_GUARD_BEGIN?

> Source/WTF/wtf/Compiler.h:461
> +#define ALLOW_UNUSED_PARAMETER_BEGIN IGNORE_WARNING_BEGIN("-Wunused-parameter")
> +#define ALLOW_UNUSED_PARAMETER_END IGNORE_WARNING_END

Seems like this should be "allow unused parameters" with an "s".

> Source/WTF/wtf/Compiler.h:471
> +#define IGNORE_RETURN_TYPE_BEGIN IGNORE_WARNING_BEGIN("-Wreturn-type")
> +#define IGNORE_RETURN_TYPE_END IGNORE_WARNING_END

The phrase "ignore return type" is not a correct description of the effect of suppressing this warning. This macro does not cause someone to ignore a return type. Maybe IGNORE_RETURN_TYPE_WARNINGS_BEGIN or ALLOW_DEFAULT_RETURN_TYPES_BEGIN?

> Source/WTF/wtf/Compiler.h:476
> +#define ALLOW_NULL_ARGUMENT_BEGIN IGNORE_WARNING_BEGIN("-Wnonnull")
> +#define ALLOW_NULL_ARGUMENT_END IGNORE_WARNING_END

The phrase "allow null argument" is not a correct description of the effect of suppressing this warning. Perhaps ALLOW_NULL_CHECKS_ON_REFERENNCES_BEGIN? Or IGNORE_NULLITY_CHECK_WARNINGS_BEGIN? Or something else like that.

> Source/WebCore/page/mac/EventHandlerMac.mm:617
> -#pragma clang diagnostic push
> -#pragma clang diagnostic ignored "-Wdeprecated-declarations"
> +ALLOW_DEPRECATED_DECLARATIONS_BEGIN
>                                    convertScreenToBase:[NSEvent mouseLocation]]
> -#pragma clang diagnostic pop
> +ALLOW_DEPRECATED_DECLARATIONS_END

Should indent this to match the code ("fakeEvent =") rather than putting it on the far left margin for consistency I think.

Although this does point out that it’s independent of the code flow, and perhaps we should consider always putting it on far left (I think not, but maybe).

[Guillaume Emont]

(In reply to Darin Adler from comment #42)
> > Source/WTF/wtf/Compiler.h:419
> > +#if !defined(IGNORE_GCC_WARNING_BEGIN) && COMPILER(GCC)
> 
> If seems that we have extra #if !defined guards. Does someone need to
> override these macros with definitions done before including this source
> file? We could add that feature (allowing an override) later if we need it;
> I am pretty sure we don’t need it and I would prefer to omit the guards
> except in cases where we use them to avoid complex #else constructions.

I used the if !defined() mainly to match the other macro definitions in the file, but I'm not particularly attached to it, so I removed it in my new version. I left the ones in the other macro definitions though, should I remove them?

I also applied all your other suggestions. Thanks for the thoughtful review!

[Guillaume Emont]

Created attachment 349427 [details]
Patch

New version applying Darin's latest suggestions, as well as replacing newly introduced pragmas after rebasing.

[EWS Watchlist]

Attachment 349427 [details] did not pass style-queue:


ERROR: Source/WebCore/xml/XPathGrammar.cpp:2157:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/xml/XPathGrammar.cpp:2160:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 205 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

(In reply to Guillaume Emont from comment #43)
> I used the if !defined() mainly to match the other macro definitions in the
> file, but I'm not particularly attached to it, so I removed it in my new
> version. I left the ones in the other macro definitions though, should I
> remove them?

Let's leave the unrelated macros unchanged in this patch.

[Michael Catanzaro]

Comment on attachment 349427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349427&action=review

I haven't reviewed every use of the macros, but the sampling of the changes I looked at seem good, and the macros themselves look good too, and have been extensively discussed here. I like! r=me

> Source/WTF/wtf/Compiler.h:452
> +#if !defined(ALLOW_DEPRECATED_DECLARATIONS_BEGIN)
> +#define ALLOW_DEPRECATED_DECLARATIONS_BEGIN IGNORE_WARNINGS_BEGIN("deprecated-declarations")
> +#define ALLOW_DEPRECATED_DECLARATIONS_END IGNORE_WARNINGS_END
> +#endif

Please get rid of all the #if !defined() checks here too. They're not useful and just clutter the file. Nobody is going to be redefining these ever.

[Darin Adler]

Comment on attachment 349427 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349427&action=review

> Source/WebKitLegacy/mac/History/WebHistory.h:134
> -#pragma clang diagnostic push
> -#pragma clang diagnostic ignored "-Wdeprecated-declarations"
> +ALLOW_DEPRECATED_DECLARATIONS_BEGIN
>  - (NSArray *)orderedItemsLastVisitedOnDay:(NSCalendarDate *)calendarDate;
> -#pragma clang diagnostic pop
> +ALLOW_DEPRECATED_DECLARATIONS_END
>  #endif

We’ll need to omit this change because it’s in a public header that is part of macOS and iOS API and the content of such headers can’t depend on internal WebKit headers like Compiler.h.

I tried to find other instances of this and did not spot any.

[Guillaume Emont]

Created attachment 349540 [details]
Patch

Patch for landing.

[EWS Watchlist]

Attachment 349540 [details] did not pass style-queue:


ERROR: Source/WebCore/xml/XPathGrammar.cpp:2157:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/xml/XPathGrammar.cpp:2160:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 204 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

Comment on attachment 349540 [details]
Patch

Be sure to use 'webkit-patch --no-review' when keeping a previous review.

[WebKit Commit Bot]

Comment on attachment 349540 [details]
Patch

Clearing flags on attachment: 349540

Committed r235935: <https://trac.webkit.org/changeset/235935>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44383630>

[Alex Christensen]

Comment on attachment 349540 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349540&action=review

> Source/WebKitLegacy/mac/Carbon/HIWebView.mm:328
> -#pragma clang diagnostic push
> +        ALLOW_DEPRECATED_DECLARATIONS_END

This "changing to what it ought to have been" broke an internal build because there are deprecated things below it.

[Alex Christensen]

http://trac.webkit.org/r235957

[Guillaume Emont]

(In reply to Alex Christensen from comment #56)
> http://trac.webkit.org/r235957

Sorry about that, I made sure to add a pair of ALLOW_DEPRECATED_DECLARATIONS_BEGIN/_END further on so that it compiles on the EWS where this created a failure. By definition, I cannot see internal build failures, but I think the proper way to fix it is to add other pairs of ALLOW_DEPRECATED_DECLARATIONS_BEGIN/END around the lines that fail on that internal build, until it doesn't fail any more ;).

[Alex Christensen]

I agree.  Someone with a setup that can reproduce that build failure should do that.
I think this patch was a great improvement.  Thanks!