900040 - consider using new C++11 type_traits to statically enforce podliness in the Pod* utility functions

Status: NEW

(Core :: MFBT, defect)

Description

[Luke Wagner [:luke]]

The PodOperations.h utility functions do memcpy/memset/etc w/o proper copy-constructor/assignment calls and thus should not be used on types with non-trivial copy/assignment.  Currently, this is not enforced (the hope is that the "Pod" in the name is suggestive enough), but with C++11 <type_trais> we could!  The obvious choice is std::is_pod<T>, but I think the actual legal definition of POD might be more strict than we need.  Instead, I think we could go with some combination of std::has_trivial_copy_constructor and std::has_trivial_assign.

Comment 1

[(no longer active)]

Again, are those available in STLport?  If not we cannot use them.

Comment 2

[Mike Hommey [:glandium]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #1)
> Again, are those available in STLport?  If not we cannot use them.

They are. The question is whether they are correct.

Comment 3

[Luke Wagner [:luke]]

Ugh.  I just looked and afaics the STLPort impl of is_pod (at least on STLPort 5.2.1) really just recognizes builtins, not user-defined POD classes.

So, just so I understand, we have some platform (which platforms?) that does have a C++11 compiler but a generic STLPort standard library?

Ultimately you can't implement std::is_pod w/o compiler support (e.g., it looks like GCC's std::is_pod calls a builtin __is_pod(T)) so it seems like we could define our own mozilla::IsPod (not so fast bug 900042!) that either uses std::is_pod for !STLPort or uses whatever the compiler provides on the platforms on which we use STLPort.

Comment 4

[Joshua Cranmer [:jcranmer]]

MSVC appears to define is_pod as follows, roughly:
  #define _IS_POD(_Ty)	: _Cat_base<is_void<_Ty>::value \
	|| is_scalar<_Ty>::value \
	|| __has_trivial_constructor(_Ty) && __is_pod(_Ty)>

As GCC and Clang share the same underlying macros, and MSVC I think reuses most of those same macros [more or less], patching STLport to use those macros where feasible sounds feasible.

Comment 5

[Luke Wagner [:luke]]

Ah, so you're saying __is_pod/__has_trivial_construct might actually be portable compiler intrinsic names?

Comment 6

[Joshua Cranmer [:jcranmer]]

(In reply to Luke Wagner [:luke] from comment #5)
> Ah, so you're saying __is_pod/__has_trivial_construct might actually be
> portable compiler intrinsic names?

There are subtle edge case differences: __is_pod(int) is false on MSVC and true on clang.

MSVC's list of macros are here: <http://msdn.microsoft.com/en-us/library/vstudio/ms177194.aspx>.
Clang's list is here: <http://clang.llvm.org/docs/LanguageExtensions.html#checks-for-type-traits>.
gcc's list is here: <http://gcc.gnu.org/onlinedocs/gcc/Type-Traits.html>.

Comment 7

[Luke Wagner [:luke]]

Ah, thanks for explaining.  Do you think then it makes sense to re-appropriate mozilla::IsPod to use std::is_pod or (in its absence) use these compiler-specific macros?  (Same for the other super-useful trais has_trivial_*.

Comment 8

[Joshua Cranmer [:jcranmer]]

The things TypeTraits.h has that we use are:
std::integral_constant, std::true_type, std::false_type
std::is_integral
std::is_floating_point
std::is_pointer
std::is_enum
std::is_arithmetic
std::is_const, std::is_volatile
std::is_pod
std::is_signed, std::is_unsigned
std::is_same
std::is_base_of
std::is_convertible
std::remove_const, std::remove_volatile, std::remove_cv
std::make_signed, std::make_unsigned
std::enable_if, std::conditional

Using type_traits requires STLport to support all of this in all circumstances (I'm assuming that STLport is the only defective C++ STL implementation here). Reading the code briefly, here is what it is missing:
std::is_integral<char16_t> (+ others that depend on this)
[Is std::is_enum correct here for edge cases?]
std::is_pod for complex cases
std::is_signed<char>, std::is_signed<char16_t> (and is_unsigned, obviously)
std::is_base_of
std::is_convertible
std::make_signed, std::make_unsigned

STLport's <type_traits> is rather less reliable than I thought it would be...

Comment 9

[Luke Wagner [:luke]]

Did recent FFOS changes drop the STLPort dependency?

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Luke Wagner [:luke] from comment #9)
> Did recent FFOS changes drop the STLPort dependency?

We still have an stlport dependency on Android, and an old libstdc++ on OS X (older than even than early versions of B2G required...), courtesy of 10.6 support.  I've been working on the libstdc++/OS X requirement this past week, and the Android dependency should follow thereafter.  We can then have a conversation about how tier-3 B2G really is. ;)