Bugzilla

(In reply to Nathan Froyd [:froydnj] from comment #8)

> (In reply to Jean-Yves Avenard [:jya] from comment #5)
> > the assumption that all caller would be doing a check first is obviously incorrect.
> 
> Doing the check before calling `value()` is the documented way of using this class.  If callers aren't following the contract, they should be fixed.  What would `value()` contain if `isValid()` is false?  (I see that we have made it contain 0 in many cases, which has probably encouraged people to rely on incorrect API usage.)

Absolutely. It should be fixed.

However, this should go after 67 has merged to beta, so we have time fixing the invalid use when rushing.

I also agree with your review comment that a DIAGNOSTIC_ASSERT would have been preferred.

(In reply to Nathan Froyd [:froydnj] from comment #8)

> (In reply to Jean-Yves Avenard [:jya] from comment #5)
> > the assumption that all caller would be doing a check first is obviously incorrect.
> 
> Doing the check before calling `value()` is the documented way of using this class.  If callers aren't following the contract, they should be fixed.  What would `value()` contain if `isValid()` is false?  (I see that we have made it contain 0 in many cases, which has probably encouraged people to rely on incorrect API usage.)

Absolutely. It should be fixed.

However, this should go after 67 has merged to beta, so we have time fixing the invalid use without rushing.

I also agree with your review comment that a DIAGNOSTIC_ASSERT would have been preferred.