[webkit-reviews] review granted: [Bug 187260] WebCore: Fix clang static analyzer warnings: Assigned value is garbage or undefined : [Attachment 344127] Patch v1

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 187260: WebCore: Fix clang static analyzer warnings: Assigned value is
garbage or undefined
https://bugs.webkit.org/show_bug.cgi?id=187260

Attachment 344127: Patch v1

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




--- Comment #6 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 344127
  --> https://bugs.webkit.org/attachment.cgi?id=344127
Patch v1

LGTM, modulo the comment on VRDisplayEvent below.

(In reply to David Kilzer (:ddkilzer) from comment #5)
> Comment on attachment 344127 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344127&action=review
> 
> >> Source/WebCore/Modules/mediastream/RTCStatsReport.h:57
> >>	      Type type;
> > 
> > Should type have a default value as well?
> 
> Yes, but there is no good "Invalid" option to use here as a default value.

And we cannot add one since it is bound to IDL.

> I'm torn between using Type::Codec (which would have a value of zero),
> adding a new "Invalid" enum, or doing nothing and letting the static
> analyzer warn when a subclass (substruct?) fails to set it.
> 
> Thoughts?

Ideally, the analyzer warning should be an error for that specific case.
If we cannot make it so, status quo seems fine to me.

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

> Source/WebCore/Modules/webvr/VRDisplayEvent.h:39
> +	   VRDisplayEventReason reason { VRDisplayEventReason::Mounted };

Looking at this one, reason is not required, so it should either be
std::optional<VRDisplayEventReason> or something like
VRDisplayEventReason::None.