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.
Created attachment 430290 [details] Patch
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.
(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.
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).
(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.
(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().
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.
(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.
(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.
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.
(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.
(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.
Created attachment 430306 [details] Patch
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?
(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.
Yes, fine to do those things later. Neither needs to be in this patch.
Created attachment 430314 [details] Patch
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].
<rdar://problem/78748101>