[Webkit-unassigned] [Bug 188598] [JSC] Remove gcc warnings on mips and armv7

[bugzilla-daemon at webkit.org]

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

--- Comment #6 from Guillaume Emont <guijemont at igalia.com> ---
(In reply to Michael Catanzaro from comment #5)
> Comment on attachment 347180 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347180&action=review
> 
> I'm not qualified to review the uses of this macro, but here are my thoughts
> on the macro itself:
> 
> > Source/JavaScriptCore/runtime/JSBigInt.cpp:1318
> > +// offsetOfData() makes sure that its return value is aligned to the size of
> > +// Digit, so even though we cast to char* for pointer arithmetics, the cast to
> > +// Digit* is properly aligned, though the compiler doesn't know about it,
> > +// therefore we disable this warning.
> 
> This should be aligned four spaces to the right.
Right, sorry for the oversight.

> 
> > Source/WTF/wtf/Compiler.h:400
> > +#if !defined(IGNORE_WARNING) && COMPILER(GCC_OR_CLANG)
> > +#define _WEBKIT_IGNORE_WARNING_STRINGIFY(a) #a
> > +#define IGNORE_WARNING(warning) do {\
> > +    _Pragma("GCC diagnostic push") \
> > +    _Pragma(_WEBKIT_IGNORE_WARNING_STRINGIFY(GCC diagnostic ignored warning)) \
> > +    } while (false)
> > +
> > +#define IGNORE_WARNING_END do {\
> > +    _Pragma("GCC diagnostic pop") \
> > +    } while (false)
> > +#endif
> 
> Wow, very nice. You should use this to reduce all the manual use of #pragma
> GCC diagnostic and #pragma clang diagnostic throughout the project.
I agree. I also think that should be a separate patch, WDYT?

> 
> Unfortunately, I think you are going to need two separate definitions here
> for Clang and GCC, IGNORE_WARNING_GCC and IGNORE_WARNING_CLANG, or you'll
> just wind up introducing new warnings about unknown pragmas when the wrong
> compiler is used. At least I'm pretty sure GCC complains if it doesn't
> recognize the warning, and it definitely complains if it sees #pragma clang.

Apparently, clang is OK with at least some gcc-formatted pragma ignores. For proof, the bots are happy with the ones in this patch (though they might not be necessary), there is also older existing code that does that (see e.g. JavaScriptCore/assembler/LinkBuffer.h), so I think it's not a problem for clang. Yet grepping through the code, there are ignores that are gcc specific or clang specific, so my proposal would be to have:
 - IGNORE_WARNING_GCC / IGNORE_WARNING_GCC_END, with same definition as IGNORE_WARNING(_END) in my patch but only defined for GCC.
 - IGNORE_WARNING_CLANG / IGNORE_WARNING_CLANG_END, with clang pragmas, only defined for clang
 - IGNORE_WARNING_GCC_OR_CLANG / IGNORE_WARNING_GCC_OR_CLANG_END with same definition as IGNORE_WARNING(_END), defined for both clang and GCC

We might be able to live without the third one if we can prove that clang doesn't get confused if we use both the GCC and CLANG macros together.

-- 
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/20180817/cf67595a/attachment-0001.html>