Benoit Jacob [:bjacob] (mostly away) Assignee
	Description • 10 years ago

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.

	Jeff Walden [:Waldo]
	Comment 1 • 10 years ago

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.

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 3 • 10 years ago

New version keeps the ugly, bool-parameter-ladden StdintTypeForSizeAndSignedness in namespace detail, and only exposes the magnificent UnsignedStdintTypeForSize to the outside world.

	Nathan Froyd [:froydnj]
	Comment 4 • 10 years ago

(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*

	Jeff Walden [:Waldo]
	Comment 5 • 10 years ago

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.

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 6 • 10 years ago

(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 {}.

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 7 • 10 years ago

(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.

	Benoit Jacob [:bjacob] (mostly away) Assignee
	Comment 8 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb1ddab163f9

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/cb1ddab163f9

	Firefox Commit Bot
	Comment 10 • 10 years ago

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