[Webkit-unassigned] [Bug 188996] Add IGNORE_WARNING_.* macros

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=188996

--- Comment #25 from Guillaume Emont <guijemont at igalia.com> ---
(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.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20180829/66192aa8/attachment-0001.html>