<html>
    <head>
      <base href="https://bugs.webkit.org/">
    </head>
    <body>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add IGNORE_WARNING_.* macros"
   href="https://bugs.webkit.org/show_bug.cgi?id=188996#c25">Comment # 25</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add IGNORE_WARNING_.* macros"
   href="https://bugs.webkit.org/show_bug.cgi?id=188996">bug 188996</a>
              from <span class="vcard"><a class="email" href="mailto:guijemont@igalia.com" title="Guillaume Emont <guijemont@igalia.com>"> <span class="fn">Guillaume Emont</span></a>
</span></b>
        <pre>(In reply to Darin Adler from <a href="show_bug.cgi?id=188996#c23">comment #23</a>)
<span class="quote">> (In reply to Guillaume Emont from <a href="show_bug.cgi?id=188996#c21">comment #21</a>)
> > As I explained in <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Add IGNORE_WARNING_.* macros"
   href="show_bug.cgi?id=188996#c18">https://bugs.webkit.org/show_bug.cgi?id=188996#c18</a> 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.</span >

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.
<span class="quote">> 
> >  - 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.</span >
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?

<span class="quote">> 
> 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.</span >

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.</pre>
        </div>
      </p>


      <hr>
      <span>You are receiving this mail because:</span>

      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>