900042 - rm IsPod in TypeTraits.h; use std::is_pod/has_trivial_assignment/etc instead

Status: NEW

(Core :: MFBT, defect)

Description

[Luke Wagner [:luke]]

IsPod is a hack so that Vector/HashTable can use memcpy/memset for types without meaningful copy/assignment.  With C++11, we now have a way to test this directly so it seems like we can remove IsPod (and probably a lot of the other pieces of TypeTraits.h).

Comment 1

[(no longer active)]

Is std::is_pod/etc available in STLport used on Android and b2g?

Comment 2

[Luke Wagner [:luke]]

(For those not following, see bug 900040)

Comment 3

[Luke Wagner [:luke]]

Wow, I was wrong in comment 0 about IsPos being ahack; it looks like it has matured a lot since the last time I saw it.  Anyhow, it still would be good to use std::is_pod if we can sort out the STLPort issue.

Comment 4

[Jeff Walden [:Waldo]]

TypeTraits.h is a partial reimplementation of <type_traits>.  If we're going to switch, we should probably just switch over to <type_traits> entirely, if we can.  As an intermediate step, we could have TypeTraits.h implement everything by delegation to <type_traits>, tho.

Comment 5

[Botond Ballo [:botond]]

I'd like to repurpose this bug to improve mozilla::IsPod to detect POD-ness (instead of requiring that users opt-in their types, as it currently does) as described in bug 900040 comment 3.

(My understanding is we can't just switch to std::is_pod because we're still using STLport on some platforms.)

Comment 6

[Luke Wagner [:luke]]

Are our STLport dependencies gone now?

Comment 7

[Nathan Froyd [:froydnj]]

Yes.  We ran into some funky issues attempting to change some of TypeTraits.h things into std::-ese, though...

Comment 8

[Botond Ballo [:botond]]

Note that std::is_pod<T> has been deprecated in the latest C++ standard draft. The supported equivalent is (is_trivial && is_standard_layout), but for many uses, including - I believe - ours, is_trivial is sufficient.

Comment 9

[Nathan Froyd [:froydnj]]

It's also worth noting that IsPod does not require T to be a complete type, whereas I believe std::is_pod does.  This distinction can cause issues for some uses of IsPod.

Comment 10

[Jeff Walden [:Waldo]]

That's only if someone's specialized IsPod, right?  You can do exactly the same thing for std::is_pod, as long as it's a user-defined type you're doing the specialization for.  Whether this is *desirable*...well, probably it shouldn't be.

Comment 11

[Nathan Froyd [:froydnj]]

I don't remember all the details, but I was attempting to use is_pod for some nsTArray changes I was twiddling with, and something like:

template<bool IsAPodThing>
struct HowToMoveThingsAround;

template<>
struct HowToMoveThingsAround<true> { ...use memcpy et al... };

template<>
struct HowToMoveThingsAround<false> { ...use constructors... };

template<typename T>
class TArray
{
  typedef HowToMoveThingsAround<std::is_pod<T>::value> ThingMover;

  ...
};

failed miserably.  (It does occur to me that HowToMoveThingsAround<false> needs a definition of the type, but for whatever reason that was not an issue here.)  IsPod more-or-less does the right thing here because it just says "false" for everything it doesn't explicitly know about, whereas is_pod makes a more complete evaluation of the type.

I guess you could specialize std::is_pod, but ISTR that specializing std:: templates is prohibited except when explicitly permitted?

Comment 12

[Jeff Walden [:Waldo]]

(In reply to Nathan Froyd [:froydnj] from comment #11)
> ISTR that specializing std::
> templates is prohibited except when explicitly permitted?

Nope, you can specialize templates as long as they depend on a user-defined type and are otherwise compatible with the template's requirements.  Long as you don't make std::is_pod<UDT> a lie, you can do it.  (But, again, probably should just avoid the need.)

http://eel.is/c++draft/namespace.std#5

(Also boy howdy is it time for us to get around to making nsTArray not use memcpy/memmove and just depend on the compiler figuring it out.  :-( )

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Is there any update here? Asking specifically about whether or not there is a version of IsPod that we can use here

Comment 14

[Botond Ballo [:botond]]

I would encourage new code to use standard type traits like std::is_trivial and std::is_standard_layout.

Unfortuantely, for the IPC use case in particular, I'm not sure what would be a useful trait:

• mozilla::Maybe passes std::is_standard_layout, thus making it ineffective for the sorts of cases we want to catch
• any type with a user-defined constructor or even a default member initializer fails std::is_trivial (and std::is_pod), and pretty much all of our IPC structs have one or the other to make sure all their fields are initialized

Comment 15

[Masatoshi Kimura [:emk]]

std::is_trivially_copyable?

Comment 16

[Jeff Walden [:Waldo]]

Note that historically, I've encountered various std::is_trivially_* that were not actually implemented on all the STLs we use. Or at least just one. The one I'm thinking of is currently used/hacked around here:

https://searchfox.org/mozilla-central/rev/278046367dab878316f60f0bd7f740cf73f3c447/js/src/gc/Statistics.cpp#792

However, I made no actual effort to future-proof that into something that would alert me when I could ultimately use the trait unconditionally, so it's possible it actually can just be used directly now.

Comment 17

[Kartikaya Gupta (email:[e-mail])]

(In reply to Jeff Walden [:Waldo] from comment #16)

Note that historically, I've encountered various std::is_trivially_* that were not actually implemented on all the STLs we use. Or at least just one. The one I'm thinking of is currently used/hacked around here:

For IPC purposes it doesn't matter if is_trivially_copyable isn't implemented in all the STLs we use. We just need it to be implemented in at least one platform that gets built in CI, since we'll just be doing a static_assert and if even one build fails as a result it's good enough.

From my reading of is_trivially_copyable it sounds like what we want. Botond, do you think it's appropriate for this purpose? If there's a handful of classes that we're currently using with PlainOldDataSerializer that don't pass is_trivially_copyable it's not big deal as I can try to make them trivially copyable or just explicitly serialize them.

Comment 18

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #17)

From my reading of is_trivially_copyable it sounds like what we want. Botond, do you think it's appropriate for this purpose?

Yup, is_trivially_copyable sounds like it should work. Seems to tick our boxes such as rejecting Maybe, and not imposing requirements on how default construction behaves.

Thanks, :emk, for the suggestion!

Comment 19

[Kartikaya Gupta (email:[e-mail])]

I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1622344 for adding the static_assert and using is_trivially_copyable. This bug seems wider in scope so I didn't want to hijack it.

Comment 20

[Simon Giesecke [:sg] [he/him]]

For example, in nsTArray (https://searchfox.org/mozilla-central/rev/9c6e7500c0015a2c60be7b1b888261d95095ce27/xpcom/ds/nsTArray.h#2392) I think std::is_trivially_copy_constructible_v could/should be used instead. I am just wondering if there are types for which mozilla::IsPod is specialized where the value of std::is_trivially_copy_constructible_v would not match. Do we need to analyze this for all types, or can we just replace the type trait?

Comment 21

[Botond Ballo [:botond]]

We should probably check the value of std::is_trivially_copy_constructible_v for types for which mozilla::IsPod is specialized. Based on a Searchfox search, there are only a handful of such types (not counting built-in types).

Comment 22

[Simon Giesecke [:sg] [he/him]]

(In reply to Botond Ballo [:botond] from comment #21)

We should probably check the value of std::is_trivially_copy_constructible_v for types for which mozilla::IsPod is specialized. Based on a Searchfox search, there are only a handful of such types (not counting built-in types).

Right, I somehow assumed there were more uses. That's a number that can be checked. Will do that, and report back here.

Comment 23

[Simon Giesecke [:sg] [he/him]]

I added corresponding assertions (see https://hg.mozilla.org/try/rev/ca81b66c923853d086973c9e38a81d4df79fec24): After resolving Bug 1626197, all types with IsPod specializations are trivially copyable, except for HashMapEntry, which is not copyable at all, but move-only.

Comment 24

[André Bargull [:anba]]

I have a local patch to remove most IsPod from js/src (except when used for the mozilla::Vector POD-specialization), so I don't think you need to worry much about HashMapEntry. (I think HashMapEntry only uses IsPod for https://searchfox.org/mozilla-central/rev/fa2df28a49883612bd7af4dacd80cdfedcccd2f6/js/src/frontend/NameCollections.h#164-165.)