[webkit-reviews] review denied: [Bug 188996] Add IGNORE_WARNING_.* macros : [Attachment 348297] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Guillaume Emont
<guijemont at igalia.com>'s request for review:
Bug 188996: Add IGNORE_WARNING_.* macros
https://bugs.webkit.org/show_bug.cgi?id=188996

Attachment 348297: Patch

https://bugs.webkit.org/attachment.cgi?id=348297&action=review




--- Comment #20 from Darin Adler <darin at apple.com> ---
Comment on attachment 348297
  --> https://bugs.webkit.org/attachment.cgi?id=348297
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.