[webkit-reviews] review denied: [Bug 54456] Optimizing lightning filter to ARM-neon SIMD instruction set : [Attachment 86762] patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Zoltan Herczeg
<zherczeg at webkit.org>'s request for review:
Bug 54456: Optimizing lightning filter to ARM-neon SIMD instruction set
https://bugs.webkit.org/show_bug.cgi?id=54456

Attachment 86762: patch
https://bugs.webkit.org/attachment.cgi?id=86762&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=86762&action=review

I think the patch is great, if Gavin has no more concerns or any of the other
folks familiar with the actual code I'd like to see it go in :-) Still some
thoughs:

> Source/WebCore/platform/graphics/filters/FELighting.cpp:461
> +    // Set lightnig arguments.

typo: lighting.

>> Source/WebCore/platform/graphics/filters/arm/FELightingNEON.cpp:205
>> +
>> +asm (
> 
> Extra space before ( in function call  [whitespace/parens] [4]

You could add "// NOLINT" here, so it stops reporting this error.

> Source/WebCore/platform/graphics/filters/arm/FELightingNEON.cpp:290
> +    // Multiply the alpha values by the X and Y matricies.

typo: matrices.

>> Source/WebCore/platform/graphics/filters/arm/FELightingNEON.cpp:455
>> +);
> 
> The parameter name "NL" adds no information, so it should be removed. 
[readability/parameter_name] [5]

Same here.

>> Source/WebCore/platform/graphics/filters/arm/FELightingNEON.h:27
>> +#ifndef FELightingNeonSIMD_h
> 
> #ifndef header guard has wrong style, please use: WTF_FELightingNEON_h 
[build/header_guard] [5]

Seems like a style check bug? Did someone already report it?

> Source/WebCore/platform/graphics/filters/arm/FELightingNEON.h:44
> +#define FLAG_POINT_LIGHT		    0x01
> +#define FLAG_SPOT_LIGHT		    0x02
> +#define FLAG_CONE_EXPONENT_IS_1	    0x04
> +
> +// Otherwise: Diffuse light.
> +#define FLAG_SPECULAR_LIGHT		    0x10
> +#define FLAG_DIFFUSE_CONST_IS_1	    0x20
> +#define FLAG_SPECULAR_EXPONENT_IS_1	    0x40

Can't we use enums for this?
enum LightingFlags (
    FlagPointLight = 1 << 0,
..
};

> Source/WebCore/platform/graphics/filters/arm/FELightingNEON.h:86
> +extern short s_FELightingConstantsForNeon[];

No way we can use a "short feLightingConstantsForNeon()" free function here?