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

[bugzilla-daemon at webkit.org]

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

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

-- 
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/78af69c0/attachment.html>