Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Placement New #17057

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

WalterBright
Copy link
Member

This implements Placement New, described in https://github.com/WalterBright/documents/blob/master/placementnew.md

@WalterBright WalterBright added Severity:Enhancement Review:WIP Work In Progress - not ready for review or pulling labels Nov 10, 2024
@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#17057"

@WalterBright WalterBright force-pushed the placementNew branch 3 times, most recently from 29a5064 to 4db86a4 Compare November 10, 2024 07:46
@thewilsonator thewilsonator added Review:Needs Changelog A changelog entry needs to be added to /changelog Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org Review:Needs Tests Severity:New Language Feature labels Nov 10, 2024
@WalterBright WalterBright force-pushed the placementNew branch 6 times, most recently from 311f2cb to fd23a02 Compare November 12, 2024 08:55
@WalterBright
Copy link
Member Author

I got it to work for some basic cases. Next comes extending it to more complex ones.

@WalterBright WalterBright force-pushed the placementNew branch 3 times, most recently from 6456c9a to c1d3813 Compare November 13, 2024 09:23
@WalterBright
Copy link
Member Author

Placement new for structs seem to be working now!

@WalterBright WalterBright force-pushed the placementNew branch 4 times, most recently from 5f00e37 to 49f4a4c Compare November 14, 2024 08:17
@Connor-GH
Copy link

Connor-GH commented Nov 15, 2024

If one desires to use classes without the GC, such as in BetterC, it's just awkward to use emplace

If i'm reading this right, this will allow classes in betterC, using a kind of "allocator argument" like in $OTHER_LANGUAGES? Once upon a time I made my own internal fork of dmd that forced betterC on all D code but of course that didn't last long because it couldn't compile Phobos.

(I should clarify, my usecase is making a kernel with betterC and so far I have had to use a hacky mixin and alias this in order to use inheritance. Also, will this unlock the door for interfaces?)

@WalterBright WalterBright force-pushed the placementNew branch 2 times, most recently from 16cdbd6 to 448abed Compare November 15, 2024 04:51
@TurkeyMan
Copy link
Contributor

I have proposed the OPTION to accept void[] in ADDITION to void[N]

I avoided that because if T is a void[] then the compiler will have to introduce a special case to have T not be the target but what it refers to being the target.

I don't follow. I don't see how there's any room for special cases here?

I realised what you meant here... this situation naturally occurs when you want to new an array though, so the case still has to be handled?
Why do you see this case as a problem?

@WalterBright
Copy link
Member Author

I am catching up after 4 days without internet or power. Seattle is a technologically advanced city with a primitive electric grid.

I consistently feel casually dismissed

I'm sorry you got that impression. I've provided a rationale for each decision.

Should I have perceived that it was forked from my initial proposal and I was cut out of it? I'll admit that I was somewhat offended to note my name's nowhere to be seen near the DIP on this. That feels a bit unfair... but perhaps that's the signal where I should have recognised that it's no longer my design.

You did indeed propose new(ptr) T(...) in email along with a couple examples.

The first line of the first post of the DIP in the n.g. on Oct 30: "Based on a suggestion by Manu Evans:"
https://www.digitalmars.com/d/archives/digitalmars/dip/development/First_Draft_Placement_New_Expression_421.html#N421
I updated the DIP Nov 18: https://github.com/WalterBright/documents/blob/master/placementnew.md

The dip is about passing an lvalue rather than a pointer.

perverting ... butchering

It's a good thing we're friends, Manu!

If you want things verbatim, write the DIP the way you want it. I wrote it the way I thought would work best. I've never seen an idea that survived into a specification without modification, like the replacement of void* with void[]. I've also never seen a specification that survived into an implementation without modification. (I rewrote the DIP after this implementation.)

You've wanted 3 things, and I want them too:

  1. __rvalue
  2. placement new
  3. extended alias

The first two now have implementations and will work for your use cases. The other aspect of placement new works for my use cases. We should be both happy about this.

but there was a lot more to it that I couldn't easily share in the initial conversation we had.

I didn't see the more, other than what I mentioned earlier. I know placement new from C++, having implemented it. I also implemented that in an earlier version of D: https://dlang.org/deprecate.html#Class%20allocators%20and%20deallocators and the implementation code for it is still present (and I made some use of it!).

From my point of view; this is my proposal...

Ok, but you've also made it clear it is not what you proposed :-)

The hinge of our disagreement is placement new cannot be made @safe and trying to make it safe with an unsafe cast doesn't fix it. The implementation won't allow its use in @safe code.

@TurkeyMan
Copy link
Contributor

The hinge of our disagreement is placement new cannot be made @safe and trying to make it safe with an unsafe cast doesn't fix it. The implementation won't allow its use in @safe code.

Not at all, the hinge of our disagreement is that accepting a T as input is absolutely unacceptable.
Accepting a T by ref is palpably worse, because it further hides the appearance that you're handling memory at all (no pointer in sight) and looks exactly like passing a value.

By writing:

T x;
new(x) T;

There's nothing you could do to make it look more like a value of T is the input to the function, which couldn't be further from the truth; the ONLY valid input is raw uninitialised memory. The only language we have to handle that concept is void.

I think if you were going to design this API, it's not possible to make the API worse than that; it communicates exactly the wrong thing, and I don't think you could inspire misunderstanding and user error more confidently if you tried.

Before new, there IS NO T... I'll die on that hill.
The language should not have any spec that encourages handling invalid values, and I reckon it will also complicate future lifetime analysis.

Yes I know you can write T x = void and then you want to hand that to placement new; I absolutely want to see the coersion from T to void[N] right in my face, so that there can be no mistake. Anyone that encounters that line of code will stop and ask themselves if the value they're coercing has definitely been destroyed prior, or assure it's uninitialised.

@nordlow
Copy link
Contributor

nordlow commented Nov 26, 2024

Could the current semantics of C++'s " Placement new" described in https://en.cppreference.com/w/cpp/language/new give insights on what type(s) that should be allowed to be passed as argument to new()?

@TurkeyMan
Copy link
Contributor

C++ accepts void*, and it does not perform any bounds checking.

@TurkeyMan
Copy link
Contributor

The trouble with the function apparently accepting T values, is that people will supply T values! They will get their new object out the other side like they expect, and (depending on the implementation of the constructor/destructor) there's a high chance their code will appear to just work. There's nothing in sight to suggest they might be making a mistake, and chances are they are just silently leaking memory or something like that.

@TurkeyMan
Copy link
Contributor

TurkeyMan commented Dec 11, 2024

Oh no!

void f(int* p) @nogc
{
    new(*p) int;
}
error : cannot use `new` in `@nogc` function `main.f`

Seems that placement new is not @nogc, probably hangover from new?
The expression needs to infer the attributes of the constructor it calls...

@TurkeyMan
Copy link
Contributor

    override void visit(NewExp e)
    {
+++     if (e.placement)
+++         return; // placement new
        if (e.member && !e.member.isNogc() && f.setGC(e.loc, null))
        {
            // @nogc-ness is already checked in NewExp::semantic
            return;
        }
        if (e.onstack)
            return;
        if (global.params.ehnogc && e.thrownew)
            return;                     // separate allocator is called for this, not the GC

        if (setGC(e, "cannot use `new` in `@nogc` %s `%s`"))
            return;
        f.printGCUsage(e.loc, "`new` causes a GC allocation");
    }

This worked for me locally...

@WalterBright
Copy link
Member Author

@TurkeyMan Added your fix

@TurkeyMan
Copy link
Contributor

Is it correct though?

@WalterBright
Copy link
Member Author

yes

@TurkeyMan
Copy link
Contributor

shall we merge this too?

@TurkeyMan
Copy link
Contributor

TurkeyMan commented Dec 16, 2024

Actually, I still vehemently object to the API... can we get some 3rd parties to add their opinions?
@atilaneves ?

@atilaneves
Copy link
Contributor

Let me talk to @WalterBright first.

@WalterBright
Copy link
Member Author

Documentation PR: dlang/dlang.org#4161

@WalterBright
Copy link
Member Author

Note: I have not added the T[] placement expression yet. I figure this PR is large enough without adding that in. Will do in a followup.

@WalterBright WalterBright removed the Review:Needs Spec PR A PR updating the language specification needs to be submitted to dlang.org label Jan 14, 2025
@thewilsonator thewilsonator removed Review:Needs Tests Review:Needs Changelog A changelog entry needs to be added to /changelog labels Jan 14, 2025
@thewilsonator
Copy link
Contributor

I presume this is no longer WIP? If so please remove the label

@WalterBright WalterBright removed the Review:WIP Work In Progress - not ready for review or pulling label Jan 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants