Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Added std::string_view<>#2309

Merged
dlang-bot merged 2 commits intodlang:masterfrom
TurkeyMan:std_string_view
Nov 9, 2018
Merged

Added std::string_view<>#2309
dlang-bot merged 2 commits intodlang:masterfrom
TurkeyMan:std_string_view

Conversation

@TurkeyMan
Copy link
Contributor

No description provided.

@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#2309"

@TurkeyMan TurkeyMan force-pushed the std_string_view branch 11 times, most recently from 20f8499 to 796137b Compare September 28, 2018 08:38
@TurkeyMan
Copy link
Contributor Author

string_view.cpp
test\stdcpp\src\string_view.cpp(1) : fatal error C1083: Cannot open include file: 'string_view': No such file or directory

Really? We're rolling ancient compilers here? :(

@rainers
Copy link
Member

rainers commented Sep 29, 2018

Really? We're rolling ancient compilers here? :(

The auto-tester builds with VS2013, string_view was added in VS2017.

@TurkeyMan
Copy link
Contributor Author

Removed the Win64 test. I'll add it back whenever the auto-tester can run it.
I'm consuming it locally for now, so it will continue to work.

@TurkeyMan
Copy link
Contributor Author

This is done. Can has review?

@TurkeyMan TurkeyMan force-pushed the std_string_view branch 2 times, most recently from 418ea36 to 65c0cbd Compare September 30, 2018 18:48
@TurkeyMan
Copy link
Contributor Author

This is super-useful, because can finally pass D-strings between D/C++ with no local hacks!

@TurkeyMan
Copy link
Contributor Author

cc: error: unrecognized command line option ‘-std=c++17’

Oh FFS... these testers are all vintage machines! >_<

We just have to trust that this one works, since CI can't run any of the tests (they work for me locally!)

@TurkeyMan TurkeyMan force-pushed the std_string_view branch 2 times, most recently from 4921101 to 2fe690c Compare November 6, 2018 08:38
@thewilsonator
Copy link
Contributor

Please rebase

@dlang-bot dlang-bot removed the Needs Rebase needs a `git rebase` performed label Nov 9, 2018
@TurkeyMan
Copy link
Contributor Author

Difficult to test, because very few CI machines support C++17, including the Windows CI machines...

@thewilsonator
Copy link
Contributor

Hmm, clearly some of them do. Why do we not have Appveyor for druntime? Then you could disable the autotester's windows builds and rely on that instead.

@TurkeyMan
Copy link
Contributor Author

I think we might just need to merge this on faith until all the CI machines get with the now.. I use it locally, people will report bugs if it doesn't work, but honestly, it will likely get VERY low usage in the short term, since it's not that common even in C++ yet.

@thewilsonator
Copy link
Contributor

Ahh I see that you haven't enabled the test.

@TurkeyMan
Copy link
Contributor Author

This is pretty critical for D interaction though; it allows you to pass string slices to/from D. This is the C++ slice ABI that people keep complaining we need to have.

@thewilsonator
Copy link
Contributor

Heh, I'd love to get
dlang/dmd#8120 merged.

@TurkeyMan
Copy link
Contributor Author

Yeah, it would be nice... but if we have std::string_view<> and std::span<>, do we really need that hack?
I guess it's still nice for C++ to link to D code, rather than the case of D linking to C++ code (where these are useful).

@TurkeyMan
Copy link
Contributor Author

Remove auto_merge for a minute, I just want to give it a good once-over, and run the tests comprehensively since I rebased.

@thewilsonator
Copy link
Contributor

Done. Ping me when you're ready.

@dlang-bot dlang-bot merged commit d891e38 into dlang:master Nov 9, 2018
@TurkeyMan TurkeyMan deleted the std_string_view branch November 9, 2018 06:49
@jacob-carlborg
Copy link
Contributor

jacob-carlborg commented Nov 9, 2018

Heh, I'd love to get dlang/dmd#8120 merged.

The problem is that the ABI for D strings and a struct with a corresponding layout doesn't match on Windows. D strings are passed in registers but not structs. So we would need to change the ABI as well in addition to the mangling for extern (C++) functions.

@TurkeyMan
Copy link
Contributor Author

Sadly, the struct ABI is less efficient than the register pair. I would discourage changing the ABI, and instead, just use some other proxy struct in the argument list instead (ie, std::string_view, or std::span?)

@jacob-carlborg
Copy link
Contributor

just use some other proxy struct in the argument list instead (ie, std::string_view, or std::span?)

Are those structs treated specially by the compiler to better optimize?

@TurkeyMan
Copy link
Contributor Author

No. They will perform the same as if you remove D's slice ABI optimisation. I'm saying, don't change the slice ABI, just use another struct instead (perhaps these).

@jacob-carlborg
Copy link
Contributor

I was thinking to change it only for extern(C++) functions.

@TurkeyMan
Copy link
Contributor Author

And extern(C)?

@jacob-carlborg
Copy link
Contributor

And extern(C)?

I didn't think of that. There's no mangling for C functions so the mangling problem doesn't exist there. But the ABI mismatch does, I guess.

@thewilsonator
Copy link
Contributor

I think most perf loss can be averted with (cross language) LTO

@TurkeyMan
Copy link
Contributor Author

VS LTO is not supported by any 3rd party toolchains.

@thewilsonator
Copy link
Contributor

Of course,grumble,grumble. It does work fine for LDC/Clang and I presume GDC/G++.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants