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

RESOLVED FIXED in mozilla22

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

Trunk
mozilla22
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

CheckedInt is used in WebKit. This patch fixes and simplifies the preprocessor stuff to keep it working without relying on the rest of MFBT.
Attachment #723269 - Flags: review?(jwalden+bmo)
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.
Attachment #723269 - Flags: review?(jwalden+bmo) → review+
(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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/7e347694edf2
Assignee: nobody → bjacob
Target Milestone: --- → mozilla22
https://hg.mozilla.org/mozilla-central/rev/7e347694edf2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.