Bug 181050 – Cleanup: Dereference value of optional directly instead of using checked value in WebCore::subdivide()

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Daniel Bates]

Cleanup: Deference value of optional directly instead of using checked value in WebCore::subdivide().

[Daniel Bates]

Created attachment 329940 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 329940 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Cleanup: Deference value of optional directly instead of using checked value in WebCore::subdivide()

Dereference

[Daniel Bates]

Committed r226195: <https://trac.webkit.org/changeset/226195>

[Radar WebKit Bug Importer]

<rdar://problem/36161451>

[Darin Adler]

This does not seem right. Not harmful, but not helpful either.

I thought that std::optional::operator* and std::optional::value are supposed to have identical semantics. The choice of which one to use should be about clarity, not runtime checking cost.

Looking at our Optional.h header, in fact it seems that currently operator* has the assertion and value does not. Which seems quite peculiar. Not sure why.

JF?

[JF Bastien]

(In reply to Darin Adler from comment #5)
> This does not seem right. Not harmful, but not helpful either.
> 
> I thought that std::optional::operator* and std::optional::value are
> supposed to have identical semantics. The choice of which one to use should
> be about clarity, not runtime checking cost.
> 
> Looking at our Optional.h header, in fact it seems that currently operator*
> has the assertion and value does not. Which seems quite peculiar. Not sure
> why.
> 
> JF?

They don't have the same semantics when the optional doesn't contain a value.

operator*() says:
  Requires: *this contains a value.

Therefore using it when the optional contains no value is UB. A valid implementation could throw or abort in these cases, or just return whatever random thing the memory contained.

Whereas value() has no requires clause, and instead says:
  Effects: Equivalent to:
    return bool(*this) ? *val : throw bad_optional_access();

Without exception support, the right thing to do in value() is to abort.


So I agree with Dan that it's sufficient to use operator* when you've proven that a value is indeed contained. I'm not sure it's more efficient: if we've proven it then presumably the compiler can eliminate the value check: since it's the same thing, that check should just go away, I'd look at the assembly. If the compiler doesn't do it I'd say it's a bug, or it hasn't been proven sufficiently :-)

Maybe our implementation of optional should be changed to abort always on bad usage?

Another pattern that's nice is unwrapping the value and then just using that. Not sure that works here? Maybe value_or() would be appropriate (though seems not either...).