Issue 9237 - Add isPOD trait #1421

ghost · 2012-12-29T00:39:32Z

http://d.puremagic.com/issues/show_bug.cgi?id=9237

Initial attempt.

ghost · 2012-12-29T00:45:13Z

Wait don't pull this yet. Those sanity checks should fail to compile instead of returning false, and the trait should only accept one parameter. Re-working..

WalterBright · 2012-12-29T00:51:59Z

that was fast!

ghost · 2012-12-29T00:58:09Z

@WalterBright: Regarding your opAssing comment in the newsgroups -- The isPOD function comment states:

no postblits, constructors, destructors, or assignment operators

Should I remove the opassign check and remove the requirement from the docstring?

ghost · 2012-12-29T01:01:48Z

I'll move the test-cases to test/runnable/traits, stay tuned.

dnadlinger · 2012-12-29T01:02:59Z

Please don't merge before the discussion on what a POD type should be is finished – this makes what mostly is an implementation detail user visible, so it would become harder to change. (typing up a NG post right now)

ghost · 2012-12-29T01:06:41Z

OK. In the meantime I've moved the test-cases to where they should be, and also I've removed the opAssign check (it was failing anyway in Phobos due to forward reference issues). I won't be doing further edits until we've agreed on the POD discussion.

WalterBright · 2012-12-29T01:13:15Z

At the moment, I suggest just matching in the test case what the compiler does. Any reengineering if what isPOD does in the compiler is a separate issue.

andralex · 2012-12-29T01:26:38Z

@WalterBright: what is the exact definition according to the compiler?

9rnsr · 2012-12-29T01:41:25Z

test/runnable/traits.d

+    // sanity checks
+    static assert(!__traits(compiles, { __traits(isPOD, C_9273); } ));
+    static assert(!__traits(compiles, { __traits(isPOD, 123); } ));
+    static assert(!__traits(compiles, { __traits(isPOD, 123, 456); } ));


I think these sanity checks are done by incorrect way.
{ __traits(isPOD, 123, 456); } will always fail to compile, because it will report has no effect in expression error.
Instead, you should write as: static assert(!__traits(compiles, __traits(isPOD, 123, 456) ));

Thanks. I've needlessly added the { } and then the compiler complained about lacking ;, which distracted me from the real issue.

ghost · 2012-12-29T01:53:29Z

Shouldn't the language dictate what a POD is? C++ defines it in the spec, and the compilers follow the spec.

dnadlinger · 2012-12-29T01:57:24Z

@andralex: As far as the compiler is concerned, this is only relevant as part of implementing the System V x86-64 ABI, which is used on most x86_64 Posixen (but notably not on Windows). See page 19 of the 0.99.6 version of the spec (the official x86-64.org site seems to be down) for the requirements. There is a certain fuzziness involved in applying this to D, but I think the current implementation (disallowing constructors) is overly restrictive – see the NG for more.

dnadlinger · 2012-12-29T02:03:41Z

@AndrejMitrovic: There isn't really a reason for defining POD in the D spec, as it doesn't influence the lanugage on a semantic level. isPOD is really just used for deciding whether a type is eligible for being passed in registers or not.

But nevertheless, yes, the exact rules for when this passing in registers happens should be defined as part of a x86_64 (and Posix) specific "implementation notes" section of the docs.

dnadlinger · 2012-12-29T02:08:26Z

@WalterBright: I agree, but it must be clear from the docs that __traits(isPOD, …) – similarly to __argTypes – is only an architecture-specific detail, and there is no further meaning attached to the term "POD type" in the language.

WalterBright · 2012-12-29T02:27:15Z

src/traits.c

+                goto Lfalse;
+        }
+        error("argument %s must be a struct type", o->toChars());
+        goto Lfalse;


All non-structs are POD.

I thought this trait was meant to be used only with a struct because it's the only D type which may or may not be a POD based on its definition, hence why a trait is necessary. For other types the POD check isn't necessary.

But how are all non-struct types PODs? Classes aren't PODs?

Are we implementing an isPOD trait for structs, or some kind of isPassedViaRegisters trait? I'm confused at what your definition of a POD is if all non-structs are PODs..

WalterBright · 2012-12-29T02:29:58Z

@andralex: https://github.com/D-Programming-Language/dmd/blob/master/src/struct.c line 763

9rnsr · 2012-12-29T02:43:51Z

Just a question: StructDeclaration::isPOP() should check hasIdentityAssign, or not?

WalterBright · 2012-12-29T02:58:19Z

All non-struct data is implicitly POD, because only structs have functions that must be called for copying, creation, and destruction. Remember, a class is POD because copying the class reference around is always "trivial".

Also consider the use of isPOD. As far as I can see, it will always be used in pairs, as in "!isStruct || isPOD" if isPOD only applies to structs. Might as well fix isPOD.

Remember, a POD type can be enregistered, if it also fits in registers and the code gen supports it. A non-POD can never be enregistered. A POD type can also be bit copied around at will without regard to needing to call any functions for any part of that. (This means POD can be spilled into registers, cached in registers, stored half in registers and half in memory, etc.)

And besides, I want to emphasize here that this pull request has nothing to do with which structs are PODs and which are not. That is a completely separate issue. I don't think that this pull request should be impaired by trying to redesign POD in the language.

dnadlinger · 2012-12-29T03:12:50Z

@WalterBright: You are right, the pull request per se doesn't have anything to do with which structs are POD and which structs are not, and the "PODness" of a type is a pretty irrelevant detail of the target ABI. I'm just afraid that this is not clear without appropriate docs (see e.g. the confusion on the NG surrounding the term POD), people might start to depend on it for completely wrong use cases, and then we are stuck with the current behavior because it is no longer an invisible low-level detail.

Also, what should isPOD return on x86 or Win64? The same as for x86_64 Posixen? If so, what if for some non-x86 target there is another ABI with different rules for struct parameter passing?

WalterBright · 2012-12-29T03:17:40Z

POD must match the C ABI of the target system. Therefore, it is possible (though I don't currently know of a case and can't think of a reason for one) that the definition of POD may change from one target to the next.

ghost · 2012-12-29T04:04:50Z

And besides, I want to emphasize here that this pull request has nothing to do with which structs are PODs and which are not.

I'm not sure what you mean by that, the isPod() call is in there.

Remember, a POD type can be enregistered..

I understand all of your points now, however there's really nothing to remember here. There is the terminology of PODs which you define verbally, and then there's the D documentation which mentions PODs several times:

POD (Plain Old Data)
    Refers to a struct that contains no hidden members, 
does not have virtual functions, does not inherit, 
has no destructor, and can be initialized and 
copied via simple bit copies. D structs are POD.

POD (Plain Old Data, as in C style) structs bring
 together related data declarations, classes bring 
together related data and function declarations...

Whereas classes are reference types, structs are
 value types. Any C struct can be exactly represented as
 a D struct, except non-static function-nested D structs 
which access the context of their enclosing scope.
 In C++ parlance, a D struct is a POD (Plain Old Data)
 type, with a trivial constructor and destructor.

How can you expect people not to be confused when the docs use one terminology and you use a different terminology?

We better clearly define what a POD is in the documentation before any pulls like this are merged.

dnadlinger · 2012-12-29T04:45:08Z

Also see dlang/dlang.org#222, which at least obliterates the quite obviously wrong statement that D structs are always POD. But in any case, the way in which the term POD comes up in the docs illustrates how it is perceived to be a "high-level" concept, whereas isPod() is merely a platform ABI implementation detail.

WalterBright · 2012-12-29T05:09:46Z

The definition of POD does not need to be correct in the documentation (I agree that needs work) in order for this to be pulled. There's a need for it right now to fix a regression:

http://d.puremagic.com/issues/show_bug.cgi?id=9099

@AndrejMitrovic, I mean no disrespect, that was just a meta comment that I just don't think this pull request needs to be diverted into a discussion of the fine points of what kinds of structs are PODs and what are not. That's an entirely separate discussion. The only relevance here would be the treatment of non-structs, which should all be regarded as POD.

Certainly, the test cases you included with the pull would change with a change in the implementation of isPOD(), but that's still irrelevant to the usefulness of this pull and the point of it.

WalterBright · 2012-12-29T05:10:30Z

which at least obliterates the quite obviously wrong statement that D structs are always POD.

That was true originally, but D structs have evolved quite a bit since that was written.

ghost · 2012-12-29T15:05:54Z

@WalterBright: Updated, non-structs are now PODs. One final question: should I disallow expressions? I take it the trait should only work on types.

WalterBright · 2012-12-29T20:24:06Z

src/traits.c

+        Type *t = isType(o);
+        StructDeclaration *sd;
+        if (t && t->toBasetype()->ty == Tstruct
+              && ((sd = (StructDeclaration *)(((TypeStruct *)t->toBasetype())->sym)) != NULL))


For the !t case, it should return an error saying that a type is expected.

I mean print an error.

ghost · 2012-12-29T20:52:59Z

@WalterBright: Fixed.

Issue 9237 - Add isPOD trait

ghost · 2012-12-29T21:26:14Z

Do you have a good wording for how to document this trait on dpl.org?

WalterBright · 2012-12-29T21:38:58Z

The wording should not attempt to define what POD is, it should just say it accepts a type as an argument and returns a boolean based on if it is POD or not. The POD text should be a link to where it is defined.

ghost · 2012-12-29T21:48:20Z

The POD text should be a link to where it is defined.

Right, but where is it defined? There's a definition in the glossary but it explicitly talks about structs as PODs and nothing else.

WalterBright · 2012-12-29T21:55:02Z

Just link to that for now.

Defining a POD is a separate issue from the traits isPOD and the description of isPOD.

ghost · 2012-12-29T21:58:47Z

dlang/dlang.org#229

dnadlinger · 2012-12-29T22:33:44Z

@WalterBright: What about static arrays of non-POD structs? At least for single-element static arrays, this is going to matter with the current DMD ABI implementation, and when/if you implement your plans of treating static arrays like structs, it will also in the general case.

WalterBright · 2012-12-29T23:42:19Z

Discussion on what a POD is should go into dlang/dlang.org#229

dnadlinger · 2012-12-29T23:47:55Z

@WalterBright: This is not a discussion about what a POD type is, but about the trait implementation – as far as I can see, it is inconsistent with what the argtypes machinery does.

WalterBright · 2012-12-30T00:08:48Z

The argtype implementation relies on isPOD, so it is completely consistent. And, to answer your question, a static array is POD if and only if the array element type is POD.

dnadlinger · 2012-12-30T00:13:02Z

@WalterBright: My previous message was not meant as a question about how static arrays should be handled ABI-wise, but rather about your statement that only structs can be non-POD, and intended as a hint that the current implementation of the trait fails to handle static arrays properly. Thus, it is not consistent with the argtypes stuff.

WalterBright · 2012-12-30T00:25:15Z

That should go into bugzilla, then.

ghost · 2012-12-30T00:26:02Z

I'll just make a fixup..

WalterBright · 2012-12-30T00:26:48Z

Wait a minute, I see what you mean, there. You're right, the current implementation of traits is wrong because it doesn't handle the static array issue.

dnadlinger · 2012-12-30T00:26:48Z

@WalterBright: Oh, I missed that the pull request has already been merged.

WalterBright · 2012-12-30T00:27:28Z

If the type is a static array, it should iterate along nextOf() until it finds the element type, then do the rest.

dnadlinger · 2012-12-30T00:28:16Z

… but only for 1-element static arrays, as the general n-element case is not implemented in argtypes yet.

ghost · 2012-12-30T00:34:47Z

0-element and 2+ element static arrays are non-POD then?

dnadlinger · 2012-12-30T00:38:16Z

https://github.com/D-Programming-Language/dmd/blob/master/src/argtypes.c#L144

9rnsr reviewed Dec 29, 2012
View reviewed changes

WalterBright reviewed Dec 29, 2012
View reviewed changes

Fixes Issue 9237 - Add isPOD trait.

d53a006

WalterBright added a commit that referenced this pull request Dec 29, 2012

Merge pull request #1421 from AndrejMitrovic/Fix9237

dad4ab1

Issue 9237 - Add isPOD trait

WalterBright merged commit dad4ab1 into dlang:master Dec 29, 2012

WalterBright added a commit that referenced this pull request Dec 29, 2012

Merge pull request #1421 from AndrejMitrovic/Fix9237

5781626

Issue 9237 - Add isPOD trait

ghost mentioned this pull request Dec 30, 2012

Fixup 9237 #1423

Closed

dnadlinger mentioned this pull request Dec 23, 2013

Trivial constructors should allow the struct to be passed as POD #2975

Closed

Issue 9237 - Add isPOD trait #1421

Issue 9237 - Add isPOD trait #1421

Conversation

ghost commented Dec 29, 2012

ghost commented Dec 29, 2012

WalterBright commented Dec 29, 2012

ghost commented Dec 29, 2012

ghost commented Dec 29, 2012

dnadlinger commented Dec 29, 2012

ghost commented Dec 29, 2012

WalterBright commented Dec 29, 2012

andralex commented Dec 29, 2012

9rnsr Dec 29, 2012

Choose a reason for hiding this comment

ghost Dec 29, 2012

Choose a reason for hiding this comment

ghost commented Dec 29, 2012

dnadlinger commented Dec 29, 2012

dnadlinger commented Dec 29, 2012

dnadlinger commented Dec 29, 2012

WalterBright Dec 29, 2012

Choose a reason for hiding this comment

ghost Dec 29, 2012

Choose a reason for hiding this comment

WalterBright commented Dec 29, 2012

9rnsr commented Dec 29, 2012

WalterBright commented Dec 29, 2012

dnadlinger commented Dec 29, 2012

WalterBright commented Dec 29, 2012

ghost commented Dec 29, 2012

dnadlinger commented Dec 29, 2012

WalterBright commented Dec 29, 2012

WalterBright commented Dec 29, 2012

ghost commented Dec 29, 2012

WalterBright Dec 29, 2012

Choose a reason for hiding this comment

WalterBright Dec 29, 2012

Choose a reason for hiding this comment

ghost commented Dec 29, 2012

ghost commented Dec 29, 2012

WalterBright commented Dec 29, 2012

ghost commented Dec 29, 2012

WalterBright commented Dec 29, 2012

ghost commented Dec 29, 2012

dnadlinger commented Dec 29, 2012

WalterBright commented Dec 29, 2012

dnadlinger commented Dec 29, 2012

WalterBright commented Dec 30, 2012

dnadlinger commented Dec 30, 2012

WalterBright commented Dec 30, 2012

ghost commented Dec 30, 2012

WalterBright commented Dec 30, 2012

dnadlinger commented Dec 30, 2012

WalterBright commented Dec 30, 2012

dnadlinger commented Dec 30, 2012

ghost commented Dec 30, 2012

dnadlinger commented Dec 30, 2012