Rename Checked::unsafeGet() to Checked::value()

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Chris Dumez]

Rename Checked::unsafeGet() to Checked::value(). The "unsafeGet" naming is confusing as this function isn't really unsafe since it will crash if the value has overflowed.

[Chris Dumez]

Created attachment 430290 [details]
Patch

[Chris Dumez]

Comment on attachment 430290 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Rename Checked::unsafeGet() to Checked::value(). The "unsafeGet" naming is confusing as this

safeGet() is used a lot less and is a bit awkward to use (due to having both a return value and an out-parameter). I think my preference would be to drop safeGet() in a follow-up and port the few call sites that use it to Checked::hasOverflowed() / Checked::value(). I don't think there I much value in having separate functions.

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #2)
> Comment on attachment 430290 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430290&action=review
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        Rename Checked::unsafeGet() to Checked::value(). The "unsafeGet" naming is confusing as this
> 
> safeGet() is used a lot less and is a bit awkward to use (due to having both
> a return value and an out-parameter). I think my preference would be to drop
> safeGet() in a follow-up and port the few call sites that use it to
> Checked::hasOverflowed() / Checked::value(). I don't think there I much
> value in having separate functions.

We should just make Checked::get() return Optional<T> instead.

[Chris Dumez]

Comment on attachment 430290 [details]
Patch

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

>>> Source/JavaScriptCore/ChangeLog:8
>>> +        Rename Checked::unsafeGet() to Checked::value(). The "unsafeGet" naming is confusing as this
>> 
>> safeGet() is used a lot less and is a bit awkward to use (due to having both a return value and an out-parameter). I think my preference would be to drop safeGet() in a follow-up and port the few call sites that use it to Checked::hasOverflowed() / Checked::value(). I don't think there I much value in having separate functions.
> 
> We should just make Checked::get() return Optional<T> instead.

I initially tried that but I did not like how the call sites looked after this. I had to add new locals to store the optional result of .get() to then use those locals to replace the hasOverflowed() checks (and then later on in the spot where the value is actually needed).

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 430290 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430290&action=review
> 
> >>> Source/JavaScriptCore/ChangeLog:8
> >>> +        Rename Checked::unsafeGet() to Checked::value(). The "unsafeGet" naming is confusing as this
> >> 
> >> safeGet() is used a lot less and is a bit awkward to use (due to having both a return value and an out-parameter). I think my preference would be to drop safeGet() in a follow-up and port the few call sites that use it to Checked::hasOverflowed() / Checked::value(). I don't think there I much value in having separate functions.
> > 
> > We should just make Checked::get() return Optional<T> instead.
> 
> I initially tried that but I did not like how the call sites looked after
> this. I had to add new locals to store the optional result of .get() to then
> use those locals to replace the hasOverflowed() checks (and then later on in
> the spot where the value is actually needed).

I see. Then maybe we should just get rid of it as you suggest.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #5)
> (In reply to Chris Dumez from comment #4)
> > Comment on attachment 430290 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=430290&action=review
> > 
> > >>> Source/JavaScriptCore/ChangeLog:8
> > >>> +        Rename Checked::unsafeGet() to Checked::value(). The "unsafeGet" naming is confusing as this
> > >> 
> > >> safeGet() is used a lot less and is a bit awkward to use (due to having both a return value and an out-parameter). I think my preference would be to drop safeGet() in a follow-up and port the few call sites that use it to Checked::hasOverflowed() / Checked::value(). I don't think there I much value in having separate functions.
> > > 
> > > We should just make Checked::get() return Optional<T> instead.
> > 
> > I initially tried that but I did not like how the call sites looked after
> > this. I had to add new locals to store the optional result of .get() to then
> > use those locals to replace the hasOverflowed() checks (and then later on in
> > the spot where the value is actually needed).
> 
> I see. Then maybe we should just get rid of it as you suggest.

To be clear, my proposal is to rename unsafeGet() to value() (this patch) and drop safeGet() in a follow-up (by using hasOverflowed() + value() instead). safeGet() is not used a lot, the common pattern in WebKit was hasOverFlowed() + unsafeGet().

[Darin Adler]

Seems to me that get() returning Optional is neat. And value() being synonym for get().value() is also neat, and (aside from ambiguities over whether the crash is a debug-only assert or a release assert, which I suppose is a *big* deal) that brings us back to renaming unsafeGet() to value(), which seems really good to me. I would also use operator* as a synonym for value() and operator! and the like, all of which could make Checked more like std::optional and WTF::Expected and fit in as a family, while still being super-safe.

[Darin Adler]

Seems to me that get() returning Optional is neat. And value() being synonym for get().value() is also neat, and (aside from ambiguities over whether the crash is a debug-only assert or a release assert, which I suppose is a *big* deal) that brings us back to renaming unsafeGet() to value(), which seems really good to me. I would also use operator* as a synonym for value() and operator! and the like, all of which could make Checked more like std::optional and WTF::Expected and fit in as a family, while still being super-safe.

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #6)
> (In reply to Ryosuke Niwa from comment #5)
> > (In reply to Chris Dumez from comment #4)
> > > I initially tried that but I did not like how the call sites looked after
> > > this. I had to add new locals to store the optional result of .get() to then
> > > use those locals to replace the hasOverflowed() checks (and then later on in
> > > the spot where the value is actually needed).
> > 
> > I see. Then maybe we should just get rid of it as you suggest.
> 
> To be clear, my proposal is to rename unsafeGet() to value() (this patch)
> and drop safeGet() in a follow-up (by using hasOverflowed() + value()
> instead). safeGet() is not used a lot, the common pattern in WebKit was
> hasOverFlowed() + unsafeGet().

One unfortunate consequence of using hasOverflowed() / value() pair is that we'd check the flag twice when the compiler fails to eliminate the check in value() for crash.

[Chris Dumez]

(In reply to Darin Adler from comment #7)
> Seems to me that get() returning Optional is neat. And value() being synonym
> for get().value() is also neat, and (aside from ambiguities over whether the
> crash is a debug-only assert or a release assert, which I suppose is a *big*
> deal) that brings us back to renaming unsafeGet() to value(), which seems
> really good to me. I would also use operator* as a synonym for value() and
> operator! and the like, all of which could make Checked more like
> std::optional and WTF::Expected and fit in as a family, while still being
> super-safe.

One other thing that is confusing is that there is currently an operator bool but it looks like so:
    explicit operator bool() const
    {
        if (UNLIKELY(this->hasOverflowed()))
            this->crash();
        return m_value;
    }

So definitely doesn't behave like an optional but rather mimics doing a bool check on the underlying value.

[Darin Adler]

I think an important part of this is removing lots of the unhelpful members. I think that bool one is particularly unhelpful! Maybe let us not add an operator! or operator bool for a while. If we scale Checked back then maybe we can improve its usefulness.

[Chris Dumez]

(In reply to Darin Adler from comment #11)
> I think an important part of this is removing lots of the unhelpful members.
> I think that bool one is particularly unhelpful! Maybe let us not add an
> operator! or operator bool for a while. If we scale Checked back then maybe
> we can improve its usefulness.

I think it is a matter of taste. Checked currently tries to mimic being an actual number. So you apply arithmetic on it directly. You can use operator! or operator bool on it and it will act as if it was a number type. The only extra bit of information is hasOverflowed().

If we follow the same model, we would add an operator that would implicitly convert the Checked to its underlying number type (crashing if overflowed) and we would likely not need value() / unsafeGet() anymore.

Of course, we could go the other way and act more like optional but that's a larger behavior change. Doing arithmetic on an "optional" would not be great IMO.

[Darin Adler]

(In reply to Chris Dumez from comment #12)
> If we follow the same model, we would add an operator that would implicitly
> convert the Checked to its underlying number type (crashing if overflowed)

I like that. Seems like we could and should go with it rather than trying to be more like std::optional.

> and we would likely not need value() / unsafeGet() anymore.

I think we would still need value() when we want to use auto rather than re-writing the type, for example, and in other cases where argument type matters. The illusion that Checked is the integer type sometimes falls apart because of C++ language rules.

[Chris Dumez]

Created attachment 430306 [details]
Patch

[Darin Adler]

Comment on attachment 430306 [details]
Patch

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

Now that you added operator T, presumably many of these value() calls could have been left out; but I noticed that some you left in and others you removed. How did you decide where to use value()? Is it only used where it’s needed to compile, or was there some other rule?

> Source/JavaScriptCore/runtime/StringPrototype.cpp:315
> +    ASSERT(totalLength <= static_cast<int>(String::MaxLength));

Can we use Checked<uint32_t, AssertNoOverflow> throughout this function instead of Checked<int, AssertNoOverflow>?

Also, what’s the point of using AssertNoOverflow instead of RecordOverflow? Is that for better efficiency?

[Chris Dumez]

(In reply to Darin Adler from comment #15)
> Comment on attachment 430306 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430306&action=review
> 
> Now that you added operator T, presumably many of these value() calls could
> have been left out; but I noticed that some you left in and others you
> removed. How did you decide where to use value()? Is it only used where it’s
> needed to compile, or was there some other rule?
> 

Honestly, I used the new operator in cases where it was obvious it would build. I did not try to use it very aggressively yet. If you’d prefer, I can do another pass and use it in every case it would build.

> > Source/JavaScriptCore/runtime/StringPrototype.cpp:315
> > +    ASSERT(totalLength <= static_cast<int>(String::MaxLength));
> 
> Can we use Checked<uint32_t, AssertNoOverflow> throughout this function
> instead of Checked<int, AssertNoOverflow>?
> 
> Also, what’s the point of using AssertNoOverflow instead of RecordOverflow?
> Is that for better efficiency?

I will check. I am a little worried about making changes that could introduce regressions in a rename patch though.

[Darin Adler]

Yes, fine to do those things later. Neither needs to be in this patch.

[Chris Dumez]

Created attachment 430314 [details]
Patch

[EWS]

Committed r278338 (238371@main): <https://commits.webkit.org/238371@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 430314 [details].

[Radar WebKit Bug Importer]

<rdar://problem/78748101>