[WIP] add extern (C++) struct Darray(T) and Dstring to simplify interop with C++ #7893

timotheecour · 2018-02-14T19:42:00Z

/cc @klickverbot, @redstar @jacob-carlborg @JohanEngelen for LDC
/cc @ibuclaw for GDC
If this is deemed useful and has a chance to get merged I can finish this PR.

WIP support for extern(C++) mangling of D arrays:

struct A(T1, T2){}
alias Tarr=A!(int, float)[];

extern(C++) {
void fun2(T)(T a){}
void fun3(Tarr a){}
}

// mangles as __Z4fun38__dsliceI1AIifEE aka fun3(__dslice<A<int, float> >) (correct)
fun3(Tarr.init); 

// mangles as: _Z4fun2I8__dsliceI1AIifEEEv8__dsliceIS1_E aka void fun2<__dslice<A<int, float> > >(__dslice<A>) which is 'not' correct (should show as __dslice<A<int, float> >)
fun2(Tarr.init);

alternatives considered

status quo (using char*) => misses length information, adds complications eg extra parameter or strlen
pragma(mangle, ...) is not a viable alternative
it'd be required on every extern(C++) declaration that uses a D array, since Fix issue 18095 - Add support for pragma(mangle) on alias declarations #7458 (pragma(mangle) on alias declarations) was abandonned due to complexity
std::string, std::vector (Addition of C++ std::string, std::vector to D druntime#1316 ) is not viable as these would require copies (not slices)
[RFC] Add the core.sentinel module druntime#2123 (Add the core.sentinel module by @marler8997) won't help with D slices in general

old (needs updating)

eg usage:

// in D
// instead of existing wrappers of the form `extern(C++) char* funLib(char* input)`
extern(C++) Dstring funLib(Dstring input){
  import std.string:toUpper;
  import std.conv:to;
  return input.toNative.toUpper.to!Dstring;
}

// in C++
char a[] = "hello world";
Dstring a2{strlen(a), a};
auto a3=funLib(a2);

see full working example here showing using such API from C++ with the function defined in D.

originally proposed here: #7870 (comment)

motivations:

Compute lazy lambda serialization #7870 (comment)

Why don't you return string here? Comparing D strings is more efficient (no \0 checking)
You already know the length of the buffer - no need to throw this valuable information away.

Quoting a conversation with Walter here:

On 12/5/2017 11:20 AM, Andrei Alexandrescu wrote:
Hi Walter, Seb noticed that the dmd code is going through unpleasant hoops caused by C-style strings. Do you think incrementally switching to D-style strings would improve the compiler? -- Andrei

Yes, but we have to be mindful of gdc and ldc both relying on the C++ interface to use dmd, and D strings don't work on that interface.
I've already converted a bunch of internal stuff to D strings where the interface to the back end was not involved.

dlang-bot · 2018-02-14T19:42:02Z

Thanks for your pull request and interest in making D better, @timotheecour! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

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.

ibuclaw · 2018-02-14T19:47:32Z

We already have this.

https://github.com/dlang/dmd/blob/master/src/dmd/globals.h#L289-L295

ibuclaw · 2018-02-14T19:48:54Z

Better yet, make more of the interface internal to D, that parts that really need it can just be extern(C++) wrappers.

timotheecour · 2018-02-14T20:05:50Z

We already have this. https://github.com/dlang/dmd/blob/master/src/dmd/globals.h#L289-L295

it's not being used at all, except for DArray<BaseClass> baseInterfaces and DArray<BaseClass*> interfaces none of which is Darray<char> ; and IIRC there's no corresponding D type (which is the one proposed in this PR); but yes, this DArray would be what's used on the C++ side wrappers using the feature proposed in this PR

Better yet, make more of the interface internal to D, that parts that really need it can just be extern(C++) wrappers.

the point of this PR is to allow replacing existing C++ wrappers:
extern(C++) const(char)* getSerialization(FuncLiteralDeclaration fld)
by this
extern(C++) Dcstring getSerialization(FuncLiteralDeclaration fld)
and not loose the size information and avoid all the conversions toStringz fromStringz

jacob-carlborg · 2018-02-14T21:21:26Z

Would be nice to have #7458 working. That would allow to just create an alias with a specific mangling to avoid the need for a wrapper, Darray.

timotheecour · 2018-02-14T21:40:54Z

@jacob-carlborg we'd still need something like:

// Dstring, Dcstring, Darray as in my PR
pragma(mangle, Dstring.mangleof); alias Dstring_=char[];
pragma(mangle, Dcstring.mangleof); alias Dcstring_=const(char)[];

// then use as follows:
extern(C++) Dcstring_ funLib(Dcstring_ input);

but that'd avoid needing the need for toNative IIUC

jacob-carlborg · 2018-02-14T21:42:30Z

Or just hard code the mangling.

timotheecour · 2018-02-14T21:53:00Z

one could also do this: allow D mangling for extern(C++) declarations to understand D arrays, and mangle the same as would the following:

template<typename T>
struct __Darray{
  size_t length;
  T* ptr;
};

with this, extern(C++) char[] void funLib(char[]) should work without any uglification of D code or need for wrapper.

JohanEngelen · 2018-02-14T22:34:56Z

Certainly useful. We already have this too in LDC: https://github.com/ldc-developers/ldc/blob/bfd412cdf9237c988f44dfc241f1f9fa7bbb0359/gen/ldctraits.h#L13-L17

Perhaps define opSlice? (also Dslice may be a better name?)

thewilsonator · 2018-03-15T08:29:33Z

Whats the status of this? I'd like to start blasting away at strlen and friends.

jacob-carlborg · 2018-03-15T14:05:43Z

Whats the status of this? I'd like to start blasting away at strlen and friends.

Yes, Walter has said that strlen must die. But I prefer to mangle const(T)[] and other arrays as DSlice!T for C++ mangling.

wilzbach · 2018-03-15T14:57:05Z

src/dmd/interop.d

+}
+
+alias Dstring = Darray!char;
+alias Dcstring = Darray!(const(char));


alias string = immutable(char)[];, (though imho it should have been const(char), but I guess it's way too late to change that.)

(resolved privately on slack https://dlang.slack.com/archives/D8ZMKRGUV/p1521139433000456)
TLDR: immutable has no C++ equivalent

The slack conversation is private, so the link isn't helpful to other people.

in short: pragma(mangle) doesn't work in alias. It has been proposed in #7458, but closed due to inactivity :/

not only that but pragma(mangle) wouldn't help with arbitrary array types even if it were implemented. this PR aims at supporting arbitrary D arrays, not just const(char)[] for example

marler8997 · 2018-03-16T03:24:59Z

src/dmd/interop.d

+}
+/
+
+extern (C++) struct Darray(T)


If you're going to create a wrapper type like this you'll probably need to do it in the same way that I've done in core.sentinel. Namely, you'll need to support implicit conversion from:

Darray!(immutable(char)) > Darray!(const(char)) Darray!char > Darray!(const(char))

You may also need to support implicit conversion to/from native D arrays, but implicit conversion between multiple types requires multiple alias this.

Take a look at core.sentinel to see how this is done.

jacob-carlborg · 2018-03-16T06:41:16Z

Looks like the PR has completely changed the implementation?

jacob-carlborg · 2018-03-16T06:48:07Z

src/dmd/cppmangle.d

+        assert(t2);
+        headOfType(t2);
+        buf.writeByte('E');
+    }


I’ve been thinking how to implement this. The idea I had was to add a new method to the Type base class called typeForMangling, which takes a mangling as its argument and returns the type that should be used for that mangling. This allows a type to pose as another type during mangling. The T[] type would need to construct a DSlice!T type during the semantic phase. In this case, typeForMangling would be called when mangling T[] and use the mangling of the new type.

[ this PR is now doing two things at once, or is this a cherry pick that is about to be merged? ]

jacob-carlborg · 2018-03-24T12:47:13Z

@timotheecour What's the status of this? It doesn't look finished.

dlang-bot added the Review:WIP Work In Progress - not ready for review or pulling label Feb 14, 2018

timotheecour mentioned this pull request Feb 14, 2018

Compute lazy lambda serialization #7870

Merged

timotheecour changed the title ~~[WIP] add extern (C++) struct Darray(T) to simplify interop with C++~~ [WIP] add extern (C++) struct Darray(T) and Dstring to simplify interop with C++ Feb 14, 2018

wilzbach reviewed Mar 15, 2018

View reviewed changes

wilzbach mentioned this pull request Mar 15, 2018

Fix issue 18095 - Add support for pragma(mangle) on alias declarations #7458

Closed

timotheecour added 3 commits March 15, 2018 12:42

add extern (C++) struct Darray(T) to simplify interop with C++

89c737f

fixup

db47c80

add visit(TypeVector t)

2c8005d

timotheecour force-pushed the pr_dstring branch from 328b39c to 2c8005d Compare March 15, 2018 23:16

timotheecour requested a review from ibuclaw as a code owner March 15, 2018 23:16

marler8997 reviewed Mar 16, 2018

View reviewed changes

jacob-carlborg reviewed Mar 16, 2018

View reviewed changes

wilzbach mentioned this pull request Mar 23, 2018

[WIP] fix issue 7804 - Allow __traits(getMember) as a BasicType #8031

Closed

jacob-carlborg mentioned this pull request Apr 3, 2018

Fix issue 18716: type const(char)[] can not be mapped to C++ #8120

Open

timotheecour closed this Apr 16, 2018

Uh oh!

[WIP] add extern (C++) struct Darray(T) and Dstring to simplify interop with C++ #7893

[WIP] add extern (C++) struct Darray(T) and Dstring to simplify interop with C++ #7893

Uh oh!

Conversation

timotheecour commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WIP support for extern(C++) mangling of D arrays:

alternatives considered

old (needs updating)

Uh oh!

dlang-bot commented Feb 14, 2018

Bugzilla references

Uh oh!

ibuclaw commented Feb 14, 2018

Uh oh!

ibuclaw commented Feb 14, 2018

Uh oh!

timotheecour commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jacob-carlborg commented Feb 14, 2018

Uh oh!

timotheecour commented Feb 14, 2018

Uh oh!

jacob-carlborg commented Feb 14, 2018

Uh oh!

timotheecour commented Feb 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JohanEngelen commented Feb 14, 2018

Uh oh!

thewilsonator commented Mar 15, 2018

Uh oh!

jacob-carlborg commented Mar 15, 2018

Uh oh!

wilzbach Mar 15, 2018

Choose a reason for hiding this comment

Uh oh!

timotheecour Mar 15, 2018

Choose a reason for hiding this comment

Uh oh!

wilzbach Mar 15, 2018

Choose a reason for hiding this comment

Uh oh!

timotheecour Mar 15, 2018

Choose a reason for hiding this comment

Uh oh!

marler8997 Mar 16, 2018

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg commented Mar 16, 2018

Uh oh!

jacob-carlborg Mar 16, 2018

Choose a reason for hiding this comment

Uh oh!

JohanEngelen Mar 16, 2018

Choose a reason for hiding this comment

Uh oh!

jacob-carlborg commented Mar 24, 2018

Uh oh!

Uh oh!

timotheecour commented Feb 14, 2018 •

edited

Loading

timotheecour commented Feb 14, 2018 •

edited

Loading

timotheecour commented Feb 14, 2018 •

edited

Loading