[webkit-reviews] review granted: [Bug 206508] Enable -Wconditional-uninitialized in WebCore project : [Attachment 388254] Patch v1

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 206508: Enable -Wconditional-uninitialized in WebCore project
https://bugs.webkit.org/show_bug.cgi?id=206508

Attachment 388254: Patch v1

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




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

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

> Source/WTF/ChangeLog:3
> +	   Enable -Wconditional-uninitialized in WebCore project

Hard to tell cost/benefit for this one. Did it spot any problems?

> Source/WTF/ChangeLog:13
> +	   * icu/unicode/utf16.h:
> +	   * icu/unicode/utf_old.h:
> +	   - Initialize `__c2` variable in various macros to workaround
> +	     a potential clang bug in -Wconditional-uninitialized. This
> +	     fixes a warning in WebCore/accessibility/AXObjectCache.cpp,
> +	     but only in external SDK builds.

Since these are headers from the ICU project, we would need to upstream the
changes to ICU. But perhaps the issue is that we have old versions of these
headers? I am not sure it’s OK to change our copies of ICU headers.

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.cpp:206
> +    unsigned vectorLength = 0;

How about initializing this to 1 instead of 0. Why is 0 a good value?

> Source/WebCore/Modules/webgpu/WHLSL/WHLSLIntrinsics.cpp:221
> +    RELEASE_ASSERT(vectorLength);

Not sure adding this is helpful, and if we just start set to 1 that would help.

> Source/WebCore/accessibility/AXObjectCache.cpp:2530
> +// FIXME: Remove IGNORE_WARNINGS macros once one of
<rdar://problem/58615489&58615391> is fixed.
> +IGNORE_WARNINGS_BEGIN("conditional-uninitialized")
>	   U16_NEXT(characterOffset.node->textContent(), offset,
characterOffset.node->textContent().length(), ch);
> +IGNORE_WARNINGS_END

Does this work? If it works then why are we also patching the ICU headers? A
patch with both changes doesn’t make logical sense to me. Maybe I am missing
something?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:1317
> +	   case CryptoAlgorithmIdentifier::None:
> +	       RELEASE_ASSERT_NOT_REACHED();
> +	       break;

Unneeded, lets not add it.

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:2352
> +	   CryptoAlgorithmIdentifier hash = CryptoAlgorithmIdentifier::None;

Please just select some actual algorithm identifier to initialize to. No need
to add a "none" case.

> Source/WebCore/crypto/CryptoAlgorithmIdentifier.h:33
> +    None = 0,

Please don’t add this.

> Source/WebCore/dom/Document.cpp:3618
> -	   double delay;
> +	   double delay = 0;
>	   String urlString;
>	   if (frame && parseMetaHTTPEquivRefresh(content, delay, urlString)) {

OK to make this change.

Best fix here is probably to change parseMetaHTTPEquivRefresh to return
Optional<X> where X is a struct holding both the delay and the URL string,
rather than using out arguments.

> Source/WebCore/rendering/svg/RenderSVGResourceGradient.cpp:199
> -	   GradientData* gradientData;
> +	   GradientData* gradientData = nullptr;
>	   if (m_savedContext && (gradientData = m_gradientMap.get(&renderer)))
{

The fact that it warns on this seems like a compiler bug.