Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: froydnj, Unassigned)

Tracking

unspecified
mozilla23
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

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++.
We need this so mfbt/Endian.h can be properly restricted to integer types.
Attachment #727912 - Flags: review?(jwalden+bmo)
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.
Attachment #727912 - Flags: review?(jwalden+bmo) → review+
While writing the examples, I thought I might as well add these classes so
that const parameters (at least) come out correctly.
Attachment #732352 - Flags: review?(jwalden+bmo)
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.
Attachment #732354 - Flags: review?(jwalden+bmo)
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.
Attachment #732352 - Flags: review?(jwalden+bmo) → review+
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?
Attachment #732354 - Flags: review?(jwalden+bmo) → review+
Review comments applied.
Attachment #732352 - Attachment is obsolete: true
Attachment #732945 - Flags: review+
I assume this is OK, but you know what they say about assuming things...
Attachment #732431 - Attachment is obsolete: true
Attachment #732948 - Flags: review?(jwalden+bmo)
Review comments applied.
Attachment #727912 - Attachment is obsolete: true
Attachment #732354 - Attachment is obsolete: true
Attachment #732949 - Flags: review+
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.  :-)
Attachment #732948 - Flags: review?(jwalden+bmo) → review+
You need to log in before you can comment on or make changes to this bug.