[webkit-reviews] review denied: [Bug 26756] Fix for warnings produced with gcc 4.3 and up : [Attachment 41914] Fixes for the warnings generated on gcc 4.3 and above

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Dave MacLachlan <dmaclach at gmail.com>'s
request for review:
Bug 26756: Fix for warnings produced with gcc 4.3 and up
https://bugs.webkit.org/show_bug.cgi?id=26756

Attachment 41914: Fixes for the warnings generated on gcc 4.3 and above
https://bugs.webkit.org/attachment.cgi?id=41914&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Fix looks OK to me.

> +	   Test: none

No need to have this in ChangeLog.

>  #if COMPILER(MSVC)
>  #define SKIP_STATIC_CONSTRUCTORS_ON_MSVC 1
> +// Due to issue in gcc 4.3/4.4
> +// http://gcc.gnu.org/bugzilla/show_bug.cgi?id=40145
> +// We get warnings about some structs defined in functions based on the
> +// visibility of their member types.
> +// Using FUNCTION_LEVEL_STRUCT_VISIBILITY cleans up the warnings.
> +#define FUNCTION_LEVEL_STRUCT_VISIBILITY
>  #else
>  #define SKIP_STATIC_CONSTRUCTORS_ON_GCC 1
> +#define FUNCTION_LEVEL_STRUCT_VISIBILITY __attribute__((visibility
("hidden")))
>  #endif

This sort of thing probably does not belong in config.h.

A great place for it would be a new file named
JavaScriptCore/wtf/CompilerWorkarounds.h. Or I think you could put it with
WARN_UNUSED_RETURN in JavaScriptCore/wtf/Platform.h or somewhere else in the
WTF code. It would also probably be fine to have CompilerWorkarounds.h be in
WebCore/platform, I guess.

It would be clearer to state that we are using this to work around a compiler
bug. You call it "issue in", which in some circles might mean this is a
workaround for a problem, but that wasn't clear to me.

review- because config.h is the wrong place for this but otherwise the patch
looks fine.