849667 - Fix/simplify CheckedInt's use-outside-of-MFBT setup

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Fix CheckedInt-use-out-of-MFBT stuff

CheckedInt is used in WebKit. This patch fixes and simplifies the preprocessor stuff to keep it working without relying on the rest of MFBT.

Comment 1

[Jeff Walden [:Waldo]]

Comment on attachment 723269 [details] [diff] [review]
Fix CheckedInt-use-out-of-MFBT stuff

Review of attachment 723269 [details] [diff] [review]:
-----------------------------------------------------------------

Some of the stuff in CheckedInt already, like IsSigned and maybe MinValue, possibly wants to be moved out to other headers, for general use.  I'm not sure how well standalone use is going to play with those desirable changes.  I guess we can cross that bridge when we get to it.

::: mfbt/CheckedInt.h
@@ +17,3 @@
>  #else
> +#  include <cassert>
> +#  include <cstdint>

This should be <stdint.h>, because <cassert>'s only guaranteed to provide std::uint32_t and all those -- not ::uint32_t.

::: mfbt/tests/TestCheckedInt.cpp
@@ +8,5 @@
>  #include <iostream>
>  #include <climits>
>  
> +#ifndef MOZ_CHECKEDINT_USE_MFBT
> +#  error MOZ_CHECKEDINT_USE_MFBT should be defined by CheckedInt.h

I think usually we enclose these in "", as a style-nit.

We might want to consider having a non-mfbt CheckedInt test if we want to be sure we're not breaking this in the future.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #1)
> Comment on attachment 723269 [details] [diff] [review]
> Fix CheckedInt-use-out-of-MFBT stuff
> 
> Review of attachment 723269 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Some of the stuff in CheckedInt already, like IsSigned and maybe MinValue,
> possibly wants to be moved out to other headers, for general use.  I'm not
> sure how well standalone use is going to play with those desirable changes. 
> I guess we can cross that bridge when we get to it.

Yes, and by all means, while it's wonderful that CheckedInt is used outside of Mozilla, that shouldn't stop us from bringing improvements to MFBT. Even if CheckedInt came to depend on other MFBT files it would still be very easy for external users to rip out those few, small parts.

> 
> ::: mfbt/CheckedInt.h
> @@ +17,3 @@
> >  #else
> > +#  include <cassert>
> > +#  include <cstdint>
> 
> This should be <stdint.h>, because <cassert>'s only guaranteed to provide
> std::uint32_t and all those -- not ::uint32_t.

Ah ok.

> 
> ::: mfbt/tests/TestCheckedInt.cpp
> @@ +8,5 @@
> >  #include <iostream>
> >  #include <climits>
> >  
> > +#ifndef MOZ_CHECKEDINT_USE_MFBT
> > +#  error MOZ_CHECKEDINT_USE_MFBT should be defined by CheckedInt.h
> 
> I think usually we enclose these in "", as a style-nit.

OK.

> 
> We might want to consider having a non-mfbt CheckedInt test if we want to be
> sure we're not breaking this in the future.

Agree, that would be good to have.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/7e347694edf2

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7e347694edf2