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

[bugzilla-daemon at webkit.org]

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

--- Comment #42 from Darin Adler <darin at apple.com> ---
Comment on attachment 348812
  --> https://bugs.webkit.org/attachment.cgi?id=348812
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348812&action=review

Patch looks like it was carefully done and I did not spot any mistakes.

I have some suggestions for tweaking the names of things.

> Source/JavaScriptCore/API/JSCallbackObject.h:161
> +    IGNORE_CLANG_WARNING_BEGIN("-Wundefined-var-template")

I think the macros should take arguments that are strings without the "-W" prefix, and the macros should add the "-W" prefix.

> Source/JavaScriptCore/runtime/ConfigFile.cpp:494
> +                IGNORE_GCC_WARNING_BEGIN("-Wstringop-truncation")

No need to use the "GCC" version here since it’s already inside #if COMPILER(GCC).

I personally prefer that we use the most exotic macro, then one with the longer name with the compiler check built in, only when it’s needed. Rather than always preferring it to help emphasize the fact that the warning is only for one compiler. We could choose either style.

> Source/WTF/wtf/Assertions.cpp:82
> -#pragma clang diagnostic push
> -#pragma clang diagnostic ignored "-Wdeprecated-declarations"
> +ALLOW_DEPRECATED_DECLARATIONS_BEGIN

This should be indented to match the code, rather than put at the far left to match where the #pragma was, to be consistent with the style you selected elsewhere. I think it’s a good style choice, and best to be consistent.

> Source/WTF/wtf/Compiler.h:419
> +#if !defined(IGNORE_GCC_WARNING_BEGIN) && COMPILER(GCC)

If seems that we have extra #if !defined guards. Does someone need to override these macros with definitions done before including this source file? We could add that feature (allowing an override) later if we need it; I am pretty sure we don’t need it and I would prefer to omit the guards except in cases where we use them to avoid complex #else constructions.

> Source/WTF/wtf/Compiler.h:446
> +#define IGNORE_WARNING_BEGIN(warning)
> +#define IGNORE_WARNING_END

I am not sure if "ignore warning" is better than "ignore warnings". After all, it ignores one specific type of warning, but any number of occurrences of the code that would warn. I think I slightly prefer the term "warnings" for these macros.

Also might be better terminology to use the term "suppress warnings" rather than "ignore warnings". Not totally sure about that one.

> Source/WTF/wtf/Compiler.h:456
> +#define ALLOW_NEW_API_BEGIN IGNORE_CLANG_WARNING_BEGIN("-Wunguarded-availability-new")
> +#define ALLOW_NEW_API_END IGNORE_CLANG_WARNING_END

I’m not sure the phrase "allow new API" is quite specific enough. Maybe someone more familiar with the exact semantics can suggest a better name. Maybe it should be ALLOW_NEW_API_WITHOUT_GUARD_BEGIN?

> Source/WTF/wtf/Compiler.h:461
> +#define ALLOW_UNUSED_PARAMETER_BEGIN IGNORE_WARNING_BEGIN("-Wunused-parameter")
> +#define ALLOW_UNUSED_PARAMETER_END IGNORE_WARNING_END

Seems like this should be "allow unused parameters" with an "s".

> Source/WTF/wtf/Compiler.h:471
> +#define IGNORE_RETURN_TYPE_BEGIN IGNORE_WARNING_BEGIN("-Wreturn-type")
> +#define IGNORE_RETURN_TYPE_END IGNORE_WARNING_END

The phrase "ignore return type" is not a correct description of the effect of suppressing this warning. This macro does not cause someone to ignore a return type. Maybe IGNORE_RETURN_TYPE_WARNINGS_BEGIN or ALLOW_DEFAULT_RETURN_TYPES_BEGIN?

> Source/WTF/wtf/Compiler.h:476
> +#define ALLOW_NULL_ARGUMENT_BEGIN IGNORE_WARNING_BEGIN("-Wnonnull")
> +#define ALLOW_NULL_ARGUMENT_END IGNORE_WARNING_END

The phrase "allow null argument" is not a correct description of the effect of suppressing this warning. Perhaps ALLOW_NULL_CHECKS_ON_REFERENNCES_BEGIN? Or IGNORE_NULLITY_CHECK_WARNINGS_BEGIN? Or something else like that.

> Source/WebCore/page/mac/EventHandlerMac.mm:617
> -#pragma clang diagnostic push
> -#pragma clang diagnostic ignored "-Wdeprecated-declarations"
> +ALLOW_DEPRECATED_DECLARATIONS_BEGIN
>                                    convertScreenToBase:[NSEvent mouseLocation]]
> -#pragma clang diagnostic pop
> +ALLOW_DEPRECATED_DECLARATIONS_END

Should indent this to match the code ("fakeEvent =") rather than putting it on the far left margin for consistency I think.

Although this does point out that it’s independent of the code flow, and perhaps we should consider always putting it on far left (I think not, but maybe).

-- 
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/20180908/3ac95640/attachment.html>