DIP 1014--Hooking D's struct move semantics--Final Review

[Johan Engelen]

On Thursday, 12 July 2018 at 10:22:33 UTC, Shachar Shemesh wrote:
> On 11/07/18 20:04, Johan Engelen wrote:
>> On Wednesday, 27 June 2018 at 07:13:14 UTC, Mike Parker wrote:
>>> DIP 1014, "Hooking D's struct move semantics", is now ready 
>>> for final review.
>> 
>> after quick read:
>> 
>> (would be much easier to do inline commenting, but it appears 
>> that's not supported)
>> 
>> ### Section "Code emitted by the compiler on move"
>> Dangerous to talk about what "code is emitted" by the 
>> compiler. I think this DIP doesn't need that, and semantics is 
>> enough. "the compiler MUST call " should be reworded, because 
>> an _actual_ call to the function should not be mandatory, 
>> because it would limit the optimizer in eliding it or inlining 
>> it (note that it will be hard to _prevent_ the optimizer from 
>> eliding/inlining the call as currently specified by the DIP).
>
> I don't draw the same conclusions from the text as you.
>
> I'm perfectly fine with specifying that nothing here is 
> mandatory if the compiler ensures that *the end effect* is as 
> if it happened.
>
> AFAIK, C++ has a standing order to that effect, and it greatly 
> simplifies documenting what you want to do.

First off: I am trying to wear a strict language lawyer hat. D 
spec is already very much ill specced which is _very_ problematic 
for language and compiler development. I am not attacking the 
proposal in order to kill it. I am merely commenting on points 
that I feel should be improved.

My point was to remove things like "compiler" and "emitted code" 
from the DIP / spec. In this case, the simple rewording can be: 
"When moving a struct's instance, an implicit call to 
__move_post_blt is inserted with both new and old instances' 
addresses as arguments."


>> 
>> ### "__move_post_blt SHOULD be defined in a manner that is 
>> compatible"
>> What does "compatible" mean?
>
> "Has the same effect as".
>
> It's a little as if you're complaining about something not 
> being explicit in one section, and again about that same thing 
> being explicit in the next. Precisely why such standing order 
> would be a good idea.

Being explicit about generated machine is not good in a language 
spec. Being explicit about the semantic meaning of something 
_is_.  ;-)
"compatible" is very vague. It may mean that just the function 
signature should match.
"has the same semantic effect" would be a much better description 
of what you want.

>> Some things should be made more explicit, such as the order of 
>> calls for compound structs.
>
> I don't think it makes sense to specify the order, except to 
> say that member's opPostMove must be called before the 
> instance's opPostMove (which the code already says).

OK, so make _that_ explicit. I think there is value in 
prescribing the precise order of moves (like for construction of 
members), such that reasoning about code becomes easier.
If you want the same semantic effect (as I wrote above), then the 
text should say that the ordering is relaxed.

>> Why "SHOULD" and not "MUST"?
>
> Precisely for the reason you stated above. So that if you want 
> to do something else, you may.

Why is that freedom needed? The freedom is already provided by 
user-defined opPostMove? I think the implicit call to 
__move_post_blt is a MUST, like calls to dtors.


>> 
>> ### "This MUST return true iff a struct or any of its members 
>> have an opPostMove defined."
>> Doesn't cover struct A containing struct B containing struct C 
>> with opPostMove. Reword e.g. into: "hasElaborateMove!S MUST 
>> return true iff `S` has an `opPostMove` defined or if 
>> hasElaborateMove!X is true for any member of S of type X.
>
> Yes, I'm sorry. I worded the spec for humans.

Please don't ;-)

> I can suggest a recursive definition:
>
> hasElaborateMove for a struct MUST return true iff at least one 
> of the following is true:
> * The struct has opPostMove defined
> * hasElaborateMove returns true for at least one of the 
> struct's members.

Great.

>> 
>> 
>> ### What is lacking is a clear list of exactly in which cases 
>> `opPostMove` will be called (needed for user-facing 
>> documentation of the function).
>
> I don't think I understand this point. Can you suggest what 
> that list might contain?

I think the language spec doesn't say when a "move" is performed? 
So I don't know when exactly the opPostMove is called. Things 
that come to mind:
* exiting from struct ctor
* std.algorithm.mutation.move
Or is it enough to define what a "move" is ? (didn't check but I 
guess the DIP already explains that)
(D's "move" is different from C++'s right? D's move after exiting 
a struct's constructor doesn't lead to a destructor call, but D's 
std.algorithm.mutation.move _does_ call the destructor of the 
moved source object.)


I now realize that the DIP is a mix between language semantic 
changes (opPostMove) and implementation suggestions/details 
("__move_post_blt"). I think it would have been clearer to split 
the two in the DIP (it's valuable to have implementation 
suggestions in addition to spec changes), but by now that's too 
late. Part of my comments stem from this mixed treatment. After 
this DIP is accepted, I think the language spec should not state 
anything related to __move_post_blt, because that's just an 
implementation detail. If that's the case, then most of my 
comments are no longer important because they comment on 
suggestions instead of spec items.

Cheers,
   Johan