Closed Bug 987274 Opened 6 years ago Closed 6 years ago

Add IntegerTypeTraits.h to MFBT for additional integer traits and helpers that don't have type_traits equivalents

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

Attachments

(1 file, 2 obsolete files)

TypeTraits.h is awesome, but is explicitly limited to features that exist in <type_traits>.

There are some integer traits and helpers currently in CheckedInt.h that I'm going to need in a dependent bug (see dependent list as soon as I've filed the next bug) so I need a place where to move these to.
Attachment #8395811 - Flags: review?(jwalden+bmo)
Blocks: 987290
Blocks: 987305
Comment on attachment 8395811 [details] [diff] [review]
Move stuff to new IntegerTypeTraits.h header

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

Punting due to comment that's going to end up in this being redone.

::: mfbt/IntegerTypeTraits.h
@@ +64,5 @@
> +  : detail::StdintTypeForSizeAndSignednessHelper<Size, Signedness>
> +{};
> +
> +template<typename IntegerType>
> +struct UnsignedType

This is MakeUnsigned in TypeTraits.h these days, same for MakeSigned.
Attachment #8395811 - Flags: review?(jwalden+bmo)
Attachment #8397313 - Attachment is patch: true
New version keeps the ugly, bool-parameter-ladden StdintTypeForSizeAndSignedness in namespace detail, and only exposes the magnificent UnsignedStdintTypeForSize to the outside world.
Attachment #8397313 - Attachment is obsolete: true
Attachment #8397313 - Flags: review?(jwalden+bmo)
Attachment #8398112 - Flags: review?(jwalden+bmo)
(In reply to Benoit Jacob [:bjacob] from comment #3)
> New version keeps the ugly, bool-parameter-ladden
> StdintTypeForSizeAndSignedness in namespace detail, and only exposes the
> magnificent UnsignedStdintTypeForSize to the outside world.

*swoon*
Comment on attachment 8398112 [details] [diff] [review]
Move stuff to new IntegerTypeTraits.h header and remove stuff that is now redundant with TypeTraits.h stuff

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

::: mfbt/CheckedInt.h
@@ -9,5 @@
>  #ifndef mozilla_CheckedInt_h
>  #define mozilla_CheckedInt_h
>  
> -// Enable relying of Mozilla's MFBT for possibly-available C++11 features
> -#define MOZ_CHECKEDINT_USE_MFBT

This old USE_MFBT stuff was there so that WebKit could import our file largely without change.  I've held off somewhat in the past doing consolidation here, because of this issue.  I guess if we're hitting issues here that want these bits exposed, we probably should make changes, tho.  Maybe WebKit people will have smart ideas for how to address this, or they can just import the extra file too.

::: mfbt/IntegerTypeTraits.h
@@ +9,5 @@
> +#ifndef mozilla_IntegerTypeTraits_h
> +#define mozilla_IntegerTypeTraits_h
> +
> +#include "mozilla/TypeTraits.h"
> +#include <stdint.h>

Blank line between.

@@ +20,5 @@
> + * StdintTypeForSizeAndSignedness returns the stdint integer type
> + * of given size (can be 1, 2, 4 or 8) and given signedness
> + * (false means unsigned, true means signed).
> + */
> +template<size_t Size, bool Signedness>

Could you add

enum TypeSignedness { TypeIsSigned, TypeIsUnsigned };

to detail and use that?  More clear than true/false, certainly.

@@ +22,5 @@
> + * (false means unsigned, true means signed).
> + */
> +template<size_t Size, bool Signedness>
> +struct StdintTypeForSizeAndSignedness
> +{};

Leave off the {} so that anything but 1/2/4/8 is a compile error.

@@ +79,5 @@
> + */
> +template<typename IntegerType>
> +struct MinValue
> +{
> +private:

At present this struct's contents should be indented two more spaces.

@@ +106,5 @@
> + */
> +template<typename IntegerType>
> +struct MaxValue
> +{
> +  static_assert(IsIntegral<IntegerType>::value, "MaxValue is only for integral types");

Same here.
Attachment #8398112 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> Comment on attachment 8398112 [details] [diff] [review]
> Move stuff to new IntegerTypeTraits.h header and remove stuff that is now
> redundant with TypeTraits.h stuff
> 
> Review of attachment 8398112 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mfbt/CheckedInt.h
> @@ -9,5 @@
> >  #ifndef mozilla_CheckedInt_h
> >  #define mozilla_CheckedInt_h
> >  
> > -// Enable relying of Mozilla's MFBT for possibly-available C++11 features
> > -#define MOZ_CHECKEDINT_USE_MFBT
> 
> This old USE_MFBT stuff was there so that WebKit could import our file
> largely without change.  I've held off somewhat in the past doing
> consolidation here, because of this issue.  I guess if we're hitting issues
> here that want these bits exposed, we probably should make changes, tho. 
> Maybe WebKit people will have smart ideas for how to address this, or they
> can just import the extra file too.

Indeed this was a nontrivial change that I should have documented. Basically, like you say, I think it should be very easy for non-Mozilla users at this point to either extract the few bits of MFBT that CheckedInt.h relies on, or replace them with their own.

> 
> ::: mfbt/IntegerTypeTraits.h
> @@ +9,5 @@
> > +#ifndef mozilla_IntegerTypeTraits_h
> > +#define mozilla_IntegerTypeTraits_h
> > +
> > +#include "mozilla/TypeTraits.h"
> > +#include <stdint.h>
> 
> Blank line between.
> 
> @@ +20,5 @@
> > + * StdintTypeForSizeAndSignedness returns the stdint integer type
> > + * of given size (can be 1, 2, 4 or 8) and given signedness
> > + * (false means unsigned, true means signed).
> > + */
> > +template<size_t Size, bool Signedness>
> 
> Could you add
> 
> enum TypeSignedness { TypeIsSigned, TypeIsUnsigned };
> 
> to detail and use that?  More clear than true/false, certainly.

OK will do for landing.

> 
> @@ +22,5 @@
> > + * (false means unsigned, true means signed).
> > + */
> > +template<size_t Size, bool Signedness>
> > +struct StdintTypeForSizeAndSignedness
> > +{};
> 
> Leave off the {} so that anything but 1/2/4/8 is a compile error.

We already get a compiler error as we try to access a member typedef in an empty struct. But I guess I might still as well omit the {}.
(In reply to Benoit Jacob [:bjacob] from comment #6)
> > @@ +20,5 @@
> > > + * StdintTypeForSizeAndSignedness returns the stdint integer type
> > > + * of given size (can be 1, 2, 4 or 8) and given signedness
> > > + * (false means unsigned, true means signed).
> > > + */
> > > +template<size_t Size, bool Signedness>
> > 
> > Could you add
> > 
> > enum TypeSignedness { TypeIsSigned, TypeIsUnsigned };
> > 
> > to detail and use that?  More clear than true/false, certainly.
> 
> OK will do for landing.

Ah, I remember now what was the problem. IsSigned<T> and IsUnsigned<T> return a bool, which CheckedInt needs to feed to StdintTypeForSizeAndSignedness. We could use a Conditional to map that bool to a Signedness, but at this point, this seems like over-engineering. Bool params are bad, but as this is local to namespace detail, I would like to plead that it's OK here. I'll land as-is but can do a follow-up if you want.
https://hg.mozilla.org/mozilla-central/rev/cb1ddab163f9
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
The following changeset is now in Firefox Nightly:

> cb1ddab163f9 Bug 987274 - Add IntegerTypeTraits.h to MFBT for additional integer traits and helpers that don't have type_traits equivalents - r=Waldo

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central
You need to log in before you can comment on or make changes to this bug.