Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Updated the STL branch #2211

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Updated the STL branch #2211

wants to merge 5 commits into from

Conversation

TurkeyMan
Copy link
Contributor

Resurrected @gchatelet's STL branch

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @TurkeyMan! 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.

Testing this PR locally

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

dub fetch digger
dub run digger -- build "master + druntime#2211"

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Jun 8, 2018

Totally depends on:
dlang/dmd#8341
https://issues.dlang.org/show_bug.cgi?id=18954
Non utf-8 strings depends on:
dlang/dmd#8342

@gchatelet
Copy link
Contributor

This is awesome that you get some time to work on this.
Wow everything is mangled correctly now. So cool!

module core.stdcpp.string;

/**
* Because of broken name mangling of extern C++, this file is only available
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is no longer relevant, or is it?

///////////////////////////////////////////////////////////////////////////////
// std::string declaration.
//
// Current caveats :
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be revisited as well.

src/core/stdcpp/string.d Outdated Show resolved Hide resolved
///////////////////////////////////////////////////////////////////////////////
// std::vector declaration.
//
// Current caveats :
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

src/core/stdcpp/vector.d Outdated Show resolved Hide resolved
src/core/stdcpp/string.d Outdated Show resolved Hide resolved
@TurkeyMan
Copy link
Contributor Author

Yeah, I've been working on the mangling the C++ compatibility in DMD for weeks!
This seems like a good test that we're reaching the goal.

PR's welcome! :P

@TurkeyMan
Copy link
Contributor Author

There's crap loads of methods missing from those classes still. And basically every ptr+len method should get an additional extern(D) + T[] overload to be useful in D.

@TurkeyMan TurkeyMan force-pushed the stdcpp branch 2 times, most recently from 01f84bb to 3f1a3e3 Compare June 8, 2018 08:19
@gchatelet
Copy link
Contributor

Yeah, I've been working on the mangling the C++ compatibility in DMD for weeks!
This seems like a good test that we're reaching the goal.

I've been there. Did you manage to get all the tests here pass?

@TurkeyMan
Copy link
Contributor Author

I didn't know they existed! You're hiding them in your fork ;)

Copy link
Member

@wilzbach wilzbach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great news!!
Not sure about it, but there might be value in following the D style naming for extern(D) methods though OTOH this might be confusing for users. What do you think?


// D helpers
alias as_array this;
extern(D) T[] as_array() { return data()[0 .. size()]; }
Copy link
Member

@wilzbach wilzbach Jun 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe camelCase

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no way! this is C++, lower_snake_case.

/**
* D header file for interaction with C++ std::string.
*
* Copyright: Copyright Guillaume Chatelet 2014 - 2015.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible maybe assign the copyright to the D Language Foundation.
CC @gchatelet

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I respected the style that was in place back then but feel free to change it to whatever is needed.


// D helpers
alias as_array this;
extern(D) T[] as_array() { return data()[0 .. size()]; }
Copy link
Member

@wilzbach wilzbach Jun 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe camelCase

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not mixing naming standards within a single class!

@wilzbach
Copy link
Member

wilzbach commented Jun 8, 2018

BTW not sure whether you are aware of it, there's also an existing PR for std.pair: #1802

@TurkeyMan
Copy link
Contributor Author

Yeah I think this is C++... lower_snake_case is what's expected.
There's no indicator that a method is extern(C++) or extern(D), especially if you use intelli-sense or whatever. I expect more local-D-inlines to appear in these for perf over time.

@Geod24
Copy link
Member

Geod24 commented Jun 8, 2018

I think we should restrict ourselves to the strict requirement for compatibility for the moment. Until someone depends on it, there's no point in beautifying it.

@TurkeyMan
Copy link
Contributor Author

'beautifying' what?

@Geod24
Copy link
Member

Geod24 commented Jun 22, 2018

Blocked by https://issues.dlang.org/show_bug.cgi?id=16479 (unless you want to merge this without tests ;) )

@atilaneves
Copy link
Contributor

I don't think the different C++ standard library implementations vary much, but it might be an ongoing effort to actually get things to work (by which I mean "not have bugs" - I'm sure that vec.push_back will work). In my opinion the only way to do this properly is through dpp since then the headers are translated on the fly.

@TurkeyMan
Copy link
Contributor Author

The implementations don't vary at all from the front-end perspective... that's the definition of 'standard' ;)
The low-level functions and struct's are completely different though. Those need to live in version blocks, then get wrapped up into a form the front-end API can make use of.

I'm not sure druntime should depend on dpp, I think it's too important to have a controversial dependency like that.
I think there's a path here where we just support the structs and extern to the low-level functions (allocation, and ABI related stuff) inside the version blocks, then the mostly-trivial front-end methods can just be implemented in D using the extern stuff.

@Geod24
Copy link
Member

Geod24 commented Jul 9, 2018

I'm not even sure this should live in Druntime TBH. It will bring its own set of maintenance issues, and I'm not sure tying those to the release schedule / requirements of druntime makes sense. We do have the C binding here, but they are used by the runtime.
I think an (officially supported) dub package would be better for everyone.

@TurkeyMan
Copy link
Contributor Author

Well... it's already there.

dub... sigh

@jacob-carlborg
Copy link
Contributor

Yeah, where does it end? Since D can interface with Objective-C, does that mean we add bindings to the whole Apple SDK as well (rhetorical question).

@jmdavis
Copy link
Member

jmdavis commented Jul 10, 2018

The C bindings that druntime has are the bindings for the OS, and IMHO, it makes a lot of sense for them to be there. Binding random third party libraries (be they C or otherwise) on the other hand, really doesn't make sense. Those should be in 3rd party packages.

As for C++'s standard library, I don't know. In principle it would be nice to have all of that tightly integrated enough that you can trivially use C++ from D, but I don't know if it makes the most sense for them to be in druntime once you start considering some of the practical issues - especially since we have not taken the approach of outright building a C++ compiler into the D compiler.

@TurkeyMan
Copy link
Contributor Author

especially since we have not taken the approach of outright building a C++ compiler into the D compiler

What issue are you trying to address, and how does that help with calling into the C++ library?
Most of the C++ library is containers, which I agree with @andralex, should probably be just as available as printf. It's not like it's a dependency on an optional lib that the user has to install.

@Geod24
Copy link
Member

Geod24 commented Jul 10, 2018

It's not like it's a dependency on an optional lib that the user has to install.

Except it is. And different system will have different implementations and/or version, which will make it all the harder to test and maintain. Over the short lifespan of extern(C++) we've already encountered some issues, for example the dual ABI (and in general, inline namespaces) are not handled correctly.

@atilaneves
Copy link
Contributor

@TurkeyMan The front-end (i.e. the interface) might not change, but the layouts might. That's also important to declare properly. And that's ignoring that there's more than one C++ version, and the interface to the standard library most definitely does change between them. Which version of std::vector should we declare? The differences are mostly additive, but not always.

Then there's the fact that the headers look differently depending on preprocessor defines. Should we have version blocks for all of those despite the combinatorial explosion? And again, for which version (the headers change as compilers get updated)?

I'm not even getting close to suggesting that dpp be part of the runtime, I'm just saying that trying to put the C++ stdlib in druntime has many, many issues.

This isn't to say it shouldn't be done. But if it is, it will be lacking.

@TurkeyMan
Copy link
Contributor Author

I appreciate every point you make. I never said it was simple, but I also think you might be slightly over-complicating it though.

All D compilers have an implied complementary C++ compiler; the D compilers extern(C++) ABI support assumes a particular complementary C++ compiler.
We should only support 'current' C++ as defined by that complementary C++ compiler, and we should assume we link against STL for that same toolchain. (no point supporting moments in STL's ABI that extern(C++) doesn't support!)

For GDC, that's easy, since a version of GDC is married to a version of GCC. For any GDC build, core.stdcpp should work with whatever GCC doing.
For LDC, I'd probably track the defaults for Clang as built against at that same LLVM revision. I think that's likely the least surprising for users.

DMD is curious; on Windows, just support what's current, ie, what MS is currently supporting, since that's what DMD tracks. Posix DMD is the only question mark... what C++ compiler is DMD most likely to be used in conjunction with? (What C++ compilers are guaranteed ABI compatible with DMD? is it DMC?), and choose that compiler. This is the only case where I can see an awkward STL version situation potentially emerging, and hopefully the number of versions remains low.

Obviously the unit tests would prove core.stdcpp works against the specified complementary C++ compiler, and nothing more, and we clearly advertise the C++ that is considered complementary to the given D compiler.

So, since extern(C++) defines a large part of the ABI support, and is tested with the compilers unit-tests, the STL support that makes use of the compiler is intrinsically attached to the compiler version that it was tested against. They are interlocked, and so they need to release together.

If core.stdcpp was a separate library, it wouldn't correctly track developments in extern(C++), people would experience ABI mismatches in a somewhat stochastic way.

@atilaneves
Copy link
Contributor

When the complimentary compilers are gcc and clang, passing -std= can possibly change the layout and interface of everything in the standard library. Only cl and dmc fix the C++ version (I'm not even 100% sure cl doesn't have an option anymore). The D declaration would have to match whatever standard version the programmer uses to compile the C++ instantiation.

For all I know the mangling changes and you'd get linker errors. I'm not sure, but I also wouldn't be surprised.

@atilaneves
Copy link
Contributor

Oh, and keeping the D declarations of C++ standard library classes up-to-date with the corresponding compiler would be painful.

@TurkeyMan
Copy link
Contributor Author

TurkeyMan commented Jul 12, 2018

Right, and we may want to support -std in the future, but I'd start by officially supporting what's current.

There are some practical truth's that help us:

  • GCC/Clang don't fuck with the STL data structures in different build configs like MSVC; the only ABI change I know of in the last decade that may affect the STL struct compatibility is RVO, but that consideration is a compiler feature, and not a problem for core.stdcpp. Do you have any reason to believe -std may affect STL struct compatibility beyond RVO?
    • Solving for this would require a compiler option like -extern_cpp_no_rvo
  • MSVC do fuck with the STL data structures, but they are not affected by anything like -std; so I propose we track the current MSVC STL.
  • If there is compelling reason to support an old MS-STL data struct layout via a version() in the future, we can worry about and debate the merit of that at that time.
    • MS are getting much better at ABI stability! Hopefully conventional wisdom from the past is out of date. 2015 and 2017 are fully ABI compatible; do you know of any live projects on older toolchains? MS are good at motivating developers to upgrade.

Oh, and keeping the D declarations of C++ standard library classes up-to-date with the corresponding compiler would be painful.

But necessary, fortunately it's very easy to unit-test!

@jacob-carlborg
Copy link
Contributor

Clang recently (6.0.0) switched from GNU++98 to GNU++14. I’m not a C++ expert but this might have changed the ABI because I don’t think the small string optimization is allowed anymore.

llvm-mirror/clang@466d8da

@TurkeyMan
Copy link
Contributor Author

That change will probably have also enabled RVO by default, which is awesome, because it will match D now ;)

@jacob-carlborg
Copy link
Contributor

jacob-carlborg commented Jul 13, 2018

Another problem is that on macOS using Clang bundled with Xcode, it's difficult to know which upstream version Apple's fork corresponds to.

$ clang --version
Apple LLVM version 9.1.0 (clang-902.0.39.2)

@TurkeyMan
Copy link
Contributor Author

Fortunately, apple's distro is so closed and proprietary, that matching what they're up to should be even simpler than MSVC. It all just seems like a big effort in unit-testing.

@jacob-carlborg
Copy link
Contributor

Fortunately, apple's distro is so closed and proprietary

Technically it's a mixed environment. Most of the Unix/Posix environment and the kernel is open source, then they build closed and proprietary code on top of that. Here's the source code for Apple's fork of Clang [1].

[1] https://opensource.apple.com/source/clang/clang-800.0.42.1/

@atilaneves
Copy link
Contributor

Oh, and keeping the D declarations of C++ standard library classes up-to-date with the corresponding compiler would be painful.

But necessary, fortunately it's very easy to unit-test!

Not necessary once dpp can handle C++ well ;)

As for everything else, you make good points. I'm still wary that this might fail in mysterious and subtle ways. The only way to properly test this will be to use all possible compilers with all versions of -std= and see what happens.

Maybe there should be a core.experimental package.

@TurkeyMan
Copy link
Contributor Author

The only way to properly test this will be to use all possible compilers with all versions of -std= and see what happens.

We're not writing a C++ compiler; I don't think there's intent to adopt C++'s -std option and comprehensively conform. I still think it would be plenty useful for each compiler to choose their 'default' or most current -std and support+test just that one.

I suspect moments where our interesting subset of the ABI are broken are rare thought, and a broad range of recent -std will just work... I just don't propose we test and promise such! We don't need to own that much responsibility.

C++ does occasionally break the user-facing API/ABI, but if we are just linking to the core allocation functions, and we must maintain a matching struct (rarely if ever changes), then we should have a pretty good chance of doing a fairly robust job. Most of the trivia functions should be implemented in D for inlining's sake, and since we have different/superior semantics anyway.

@TurkeyMan TurkeyMan force-pushed the stdcpp branch 2 times, most recently from 10c9bfb to 34b05dc Compare August 5, 2018 08:47
@Geod24
Copy link
Member

Geod24 commented Jan 2, 2019

Do you want to keep this open, given that we've been merging smaller-sized PRs over the past months ?

@thewilsonator
Copy link
Contributor

Yes, I'd like to keep it open until all of its contents have been merged, its much easier to find an open PR than a closed one..

@dlang-bot dlang-bot added Needs Rebase needs a `git rebase` performed Needs Work stalled labels May 27, 2021
@RazvanN7
Copy link
Contributor

RazvanN7 commented Nov 9, 2021

@thewilsonator what is the status on this? Can we close this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Needs Rebase needs a `git rebase` performed Needs Work stalled
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants