853646 - add IsIntegral to MFBT

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Nathan Froyd [:froydnj]]

Review comments in bug 798172 comment 23 said:

> So, right now everything's purely T-generic, which seems a bit generous.  There's really > no safe way to encode or decode anything that's not a fixed-size {u,}int{8,16,32,64}_t.  > So I think we want to restrict these to taking *only* those types, and no others.  It's a > little messy to write out, but it shouldn't be too bad.

I contend that we really want IsIntegral instead as something more basic and also closer to standard C++.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: add IsIntegral to TypeTraits.h

We need this so mfbt/Endian.h can be properly restricted to integer types.

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 727912 [details] [diff] [review]
add IsIntegral to TypeTraits.h

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

I had a patch to do this in my tree, that did it with a little more complication.  I handled ignoring cv-qualifiers, and I handled signed and unsigned types separately, with some helper testing.  Also bool/char/wchar_t got handled slightly separately.  Those extra bits will be useful at some point, and probably I'll land my changes atop what you're doing eventually.  But if you don't need 'em, it's not important holding this up.  (Plus my patch required a few other changes/additions, so couldn't just land immediately.  I should probably file a bug for those bits so others can work with them, and so my queue doesn't bitrot with changes to this file.)

::: mfbt/TypeTraits.h
@@ +243,5 @@
>  template<typename T> struct IsPod<T*>       : TrueType {};
>  
>  /**
> + * IsIntegral tests whether a given type is an integral type.
> + */

Put in some examples here, like so:

+ * mozilla::IsIntegral<int>::value is true;
+ * mozilla::IsIntegral<const unsigned long>::value is true;
+ * mozilla::IsIntegral<bool>::value is true;
+ * mozilla::IsIntegral<int*>::value is false.

Also add something like this: "Note that char16_t and char32_t aren't currently handled by this because we don't know if they exist as types (whether distinct or not)."  C++11 says is_integral should be true_type for those, and we'll probably confuse people if we don't make clear to users that they're false.

@@ +245,5 @@
>  /**
> + * IsIntegral tests whether a given type is an integral type.
> + */
> +template <typename T>
> +struct IsIntegral : public FalseType {};

Don't add a "public" here -- that you're specifying the base class of a "struct"-tagged class implies public access.  This is a little obscure, but used throughout this file it makes things more readable, I think.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 0 - add mozilla::Remove{_Const,_Volatile,CV} to TypeTraits.h

While writing the examples, I thought I might as well add these classes so
that const parameters (at least) come out correctly.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add mozilla::IsIntegral to TypeTraits.h

Applied review comments and made things use RemoveCV; asking for re-review
to make sure everything is as you like it.  Should be an easy review.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 732352 [details] [diff] [review]
part 0 - add mozilla::Remove{_Const,_Volatile,CV} to TypeTraits.h

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

Basically fine enough as it goes.  Contents of the structs should be indented two further, and |Type| for the typedef as it's a type.  You wouldn't have known about it, but we had this in the JS engine already, so those uses should go away/change as well.  I actually had a patch for all of these but never had a need for it; I'll post it to point out the places that need changing.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Alternative RemoveConst/RemoveVolatile/RemoveCV patch

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 732354 [details] [diff] [review]
part 1 - add mozilla::IsIntegral to TypeTraits.h

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

::: mfbt/TypeTraits.h
@@ +78,5 @@
> + * mozilla::IsIntegral<char16_t>::value is false;
> + * mozilla::IsIntegral<char32_t>::value is false.
> + *
> + * We don't currently handle char16_t and char32_t because we don't know
> + * whether they exist as types (whether distinct or not).

Hmm.  In case there are compilers where these aren't distinct types, we probably shouldn't promise anything about what IsIntegral will do to them.  Maybe say they're unspecified, rather than false?

Comment 8

[Nathan Froyd [:froydnj]]

Attachment: part 0 - add mozilla::Remove{Const,Volatile,CV} to TypeTraits.h

Review comments applied.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 0a - remove js::StripConst and use mozilla::RemoveConst instead

I assume this is OK, but you know what they say about assuming things...

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add mozilla::IsIntegral to TypeTraits.h

Review comments applied.

Comment 11

[Jeff Walden [:Waldo]]

Comment on attachment 732948 [details] [diff] [review]
part 0a - remove js::StripConst and use mozilla::RemoveConst instead

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

rs=me assuming this is all the places.  I'm not sure it was truly necessary for me to see this, but whatever.  :-)

Comment 12

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4faeb3a2aa24
https://hg.mozilla.org/integration/mozilla-inbound/rev/349a8651db24
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab825d9e6d16

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4faeb3a2aa24
https://hg.mozilla.org/mozilla-central/rev/349a8651db24
https://hg.mozilla.org/mozilla-central/rev/ab825d9e6d16