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

Added std::string #2310

Merged
merged 7 commits into from Jul 29, 2019

Conversation

@TurkeyMan
Copy link
Contributor

commented Sep 23, 2018

This is still WIP, but the MS implementation is fully working and usable.

This is blocked on:

@dlang-bot

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2018

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 coverage diff by visiting the details link of the codecov check)
  • 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#2310"
@TurkeyMan TurkeyMan force-pushed the TurkeyMan:std_string branch 3 times, most recently from 1d1d478 to e5bcd61 Sep 23, 2018
*/
extern(C++, class) struct allocator(T)
{
static assert(!is(T == const), "The C++ Standard forbids containers of const elements because allocator!(const T) is ill-formed.");

This comment has been minimized.

Copy link
@Geod24

Geod24 Sep 25, 2018

Member

Should also check for immutable no ?

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Sep 26, 2018

Author Contributor

Sure. What I really care about though, is working out how to run those tests in this pr to prove it works on all those build machines...

This comment has been minimized.

Copy link
@Geod24

Geod24 Sep 26, 2018

Member

I'm pretty sure it does not work on POSIX, because template mangling is broken.
But having those tests running on Windows would be awesome.

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Sep 26, 2018

Author Contributor

We need to run it on every target, then I'd know that template mangling is broken and fix it...
I'm not aware of problems with template mangling. This needs to work everywhere. Can't know that unless ci executes the tests.

This comment has been minimized.

Copy link
@thewilsonator

thewilsonator Sep 26, 2018

Contributor

immutable should already be caught by the mangling code, because there is no equivalent.

This comment has been minimized.

Copy link
@Geod24

Geod24 Sep 27, 2018

Member

That's right, however the additional check is a small price to pay here. A casual reader might not realize that immutable is disallowed by the compiler. Also, we have no guarantee this won't change in the future. Lastly, it will provide a better error message.

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Sep 27, 2018

Author Contributor

These are all great ideas, but it's all worthless unless we find a way to test it ;)

This comment has been minimized.

Copy link
@rainers

rainers Sep 27, 2018

Member

See TurkeyMan#1. I suspect it will conflict with master due to other added tests, though.

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Sep 27, 2018

Author Contributor

@rainers Thanks! I can't see how the .cpp file gets built in there though, that's the trouble point, since it needs to compile for all targets, and that includes DMC and MSVC on windows...

This comment has been minimized.

Copy link
@rainers

rainers Sep 27, 2018

Member

I can't see how the .cpp file gets built in there though, that's the trouble point, since it needs to compile for all targets, and that includes DMC and MSVC on windows...

The amended changes to the makefile now contain the cpp-compilation, but as string.d doesn't compile on anything but win64, the auto-tester stops before it schedules anything to the Windows machines. I'd suggest adding version(CRuntime_Microsoft): at the top for now.

When this works to some extend, adding makefiles for other targets to the test should be straight-forward.

@TurkeyMan TurkeyMan force-pushed the TurkeyMan:std_string branch 6 times, most recently from 6c100f5 to bbf803f Sep 27, 2018
mak/COPY Show resolved Hide resolved
test/stdcpp/win64.mak Outdated Show resolved Hide resolved
@TurkeyMan TurkeyMan force-pushed the TurkeyMan:std_string branch 2 times, most recently from d4877e4 to e2863a3 Sep 28, 2018
src/core/stdcpp/string.d Outdated Show resolved Hide resolved
@TurkeyMan TurkeyMan force-pushed the TurkeyMan:std_string branch 4 times, most recently from a77324e to b3f988e Sep 30, 2018
else version (CppRuntime_Gcc)
{
///
this(DefaultConstruct) { _M_p = _M_local_data(); _M_set_length(0); }

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Sep 30, 2018

Author Contributor

@WalterBright @andralex And there it is... GNU's STL keeps an interior pointer in std::string >_<
This is the only STL implementation I've seen so far that does (I've seen a lot now!), but I'm effectively blocked on this here.
How's DIP 1014 coming along? That's probably the only way to move this forward.

This comment has been minimized.

Copy link
@thewilsonator

thewilsonator Sep 30, 2018

Contributor

If you don't need to support it, I'd say defer adding the CppRuntime_Gcc implementation. Note that because there is no instantiations of the template in druntime, you can probably just static assert(0, "CppRuntime_Gcc not yet supported");

If you do... you'll need to wait until DIP1014 is implemented.

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Sep 30, 2018

Author Contributor

I tickled my own needs months ago... I don't need to do all this work to make the tests work and hit all platforms ;)
I just want to put the lid on this whole thing... it's gone on way too long.

This comment has been minimized.

Copy link
@andralex

andralex Oct 2, 2018

Member

This is a major problem. AFAIK nobody is working on DIP 1014.

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Oct 2, 2018

Author Contributor

Well, it's sitting on your review queue... is there something wrong with it? Are you planning on rejecting it?
It's literally the thing on your desk right now isn't it? (according to the DIP schedule)

This comment has been minimized.

Copy link
@thewilsonator

thewilsonator Oct 2, 2018

Contributor

I assume he means implementation, but it is ambiguous.

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Oct 2, 2018

Author Contributor

DIP 1014 looks simple. I'm sure it'll appear promptly if it's approved.

This comment has been minimized.

Copy link
@andralex

andralex Oct 2, 2018

Member

I meant internal pointers in std::string are a major problem, and that nobody is working on DIP 1014.

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Oct 2, 2018

Author Contributor

Right, but DIP 1014 is resting with you and Walter is it not? You are the only one that can move that along.

This comment has been minimized.

Copy link
@12345swordy

12345swordy Nov 8, 2018

@TurkeyMan DIP 1014 have been recently approved

@TurkeyMan TurkeyMan force-pushed the TurkeyMan:std_string branch from b3f988e to f383e6f Sep 30, 2018
@TurkeyMan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2018

Is mangling issues?
./generated/linux/debug/64/string_cpp.o should surely have those symbols...

@thewilsonator

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2018

@TurkeyMan it would if you link the C++ std lib to it ;)

@TurkeyMan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2018

Those symbols are all templates... they should have been instantiated by string.cpp, and contained in the .o file? How can template instantiations live in the std lib?

@thewilsonator

This comment has been minimized.

Copy link
Contributor

commented Oct 2, 2018

You're missing a makefile for non-windows platforms so I guess the tester is just recursing into test/stdcpp/src/ and building .d files, ignorant of the fact that the .cpp need to be built as well.

@TurkeyMan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 2, 2018

test/stdcpp/Makefile is in the patch. That triggers the build. You can see the .cpp compile in the build log.

@TurkeyMan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Typical, there's exactly one machine in the farm running VS2013, which is very old... so I need to detect and support it.

@thewilsonator thewilsonator added auto-merge and removed Blocked labels Jul 29, 2019
@thewilsonator

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

Lets give this a little bit of testing priority...

@TurkeyMan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

I'm rebasing, since the last merge collides.

@TurkeyMan TurkeyMan force-pushed the TurkeyMan:std_string branch from e80248f to 4ac352f Jul 29, 2019
@dlang-bot dlang-bot removed the auto-merge label Jul 29, 2019
@TurkeyMan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

This is working, the test just depends on getting an environment variable into make >_<

win64.mak Outdated
@@ -108,7 +108,8 @@ test_hash:
"$(DMD)" -m$(MODEL) -conf= -Isrc -defaultlib=$(DRUNTIME) -run test\hash\src\test_hash.d

test_stdcpp:
"$(MAKE)" -f test\stdcpp\win64.mak "DMD=$(DMD)" MODEL=$(MODEL) "VCDIR=$(VCDIR)" DRUNTIMELIB=$(DRUNTIME) "CC=$(CC)" test
setmscver.bat
"$(MAKE)" -f test\stdcpp\win64.mak "DMD=$(DMD)" MODEL=$(MODEL) "VCDIR=$(VCDIR)" DRUNTIMELIB=$(DRUNTIME) "CC=$(CC)" "_MSC_VER=$(_MSC_VER)" test

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Jul 29, 2019

Author Contributor

This is the CI issue:
setmscver.bat sets the _MSC_VER variable, but then when make executes the next line (make), it seems like the variable is forgotten...

This comment has been minimized.

Copy link
@wilzbach

wilzbach Jul 29, 2019

Member

It's not forgotten. You simply execute different processes that fork from the parent environment. Try e.g. &&

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Jul 29, 2019

Author Contributor

child processes should inherit the parent environment?

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Jul 29, 2019

Author Contributor

I can't find an incantation that works... how are you supposed to do this? I need to set a make variable from a thing that a script determines?

This comment has been minimized.

Copy link
@rainers

rainers Jul 29, 2019

Member

setmscver.bat && "$(MAKE)" "_MSC_VER=%_MSC_VER%" should work.

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Jul 29, 2019

Author Contributor

it doesn't... i tried every conceivable combination of stuff like that I could think of.
I found a solution though.

But there's another problem; I just realised that windows make isn't actually make... It's an incomplete make clone!

It won't let me do ifeq in the makefile... what's that about? How do I set a variable conditionally?

This comment has been minimized.

Copy link
@rainers

rainers Jul 29, 2019

Member

It's Digital Mars make, a very basic make with some surprising behavior. (see https://dlang.org/blog/2018/06/11/dasbetterc-converting-make-c-to-d/)

How do I set a variable conditionally?

I don't think that is possible directly. Depending on the problem invoking make recursively might be part of a solution.

@TurkeyMan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

ffs, something else took the win64 runner; so we'll have to wait an hour again >_<

@thewilsonator

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

It didn't it just took a while to come online.

@TurkeyMan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

while our lives slowly drift away...

@TurkeyMan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

slowly.

@dlang-bot dlang-bot removed the auto-merge label Jul 29, 2019
if (count == 0)
return null;
T* mem;
if ((size_t.max / T.sizeof < count) || (mem = __cpp_new(count * T.sizeof)) == 0)

This comment has been minimized.

Copy link
@thewilsonator

thewilsonator Jul 29, 2019

Contributor
src\core\stdcpp\allocator.d(52): Error: cannot implicitly convert expression `__cpp_new(count * 4LU)` of type `void*` to `int*`
test\stdcpp\src\allocator_test.d(17): Error: template instance `core.stdcpp.allocator.allocator!int` error instantiating
src\core\stdcpp\allocator.d(52): Error: cannot implicitly convert expression `__cpp_new(count * 8LU)` of type `void*` to `double*`
test\stdcpp\src\allocator_test.d(18): Error: template instance `core.stdcpp.allocator.allocator!double` error instantiating
src\core\stdcpp\allocator.d(52): Error: cannot implicitly convert expression `__cpp_new(count * 24LU)` of type `void*` to `MyStruct*`
test\stdcpp\src\allocator_test.d(19): Error: template instance `core.stdcpp.allocator.allocator!(MyStruct)` error instantiating
Suggested change
if ((size_t.max / T.sizeof < count) || (mem = __cpp_new(count * T.sizeof)) == 0)
if ((size_t.max / T.sizeof < count) || (mem = cast(T*)__cpp_new(count * T.sizeof)) is null)

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Jul 29, 2019

Author Contributor

I can see that ;) ... i just had to wait 2 hours to get the error message!

This comment has been minimized.

Copy link
@rainers

rainers Jul 29, 2019

Member

I can see that ;) ... i just had to wait 2 hours to get the error message!

If you cannot test locally, maybe it would be more productive to setup your own CI on appveyor, you can select an image with VS2013 installed, too (see top of appveyor.xml). You can probably set it up before next iteration here is done...

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Jul 29, 2019

Author Contributor

I pushed before going to work. if it fails again, i'll do that tonight.
I had no idea appveyor had testers for 2013 still. we should just use it for all windows builds and get rid of the win32 auto-tester machines.

Look at the auto-tester logs; most machines take 5-10 minutes, except the windows machines which take 30-50 miunutes...

@TurkeyMan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Done!

I don't think those buildkite issues are related to this PR...

@TurkeyMan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Almost 1 year in the making!
What a fucking marathon! >_<

@andralex andralex merged commit a370406 into dlang:master Jul 29, 2019
6 checks passed
6 checks passed
CyberShadow/DAutoTest Documentation OK (154892 additions, 462 deletions)
Details
auto-tester Pass: 10
Details
buildkite/druntime Build #2177 passed (1 hour, 35 minutes, 48 seconds)
Details
ci/circleci: build Your tests passed on CircleCI!
Details
codecov/patch Coverage not affected when comparing 9e3198b...ecadacc
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@TurkeyMan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Hooolyyy smokes. That happened.
Now let's never speak of it again!

@TurkeyMan TurkeyMan deleted the TurkeyMan:std_string branch Jul 29, 2019
@@ -0,0 +1,3 @@
Added `core.stdcpp.string`.

Added `core.stdcpp.string`, which links against C++ `std::string`

This comment has been minimized.

Copy link
@wilzbach

wilzbach Jul 30, 2019

Member

Should have stated that it's only for Windows :/

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Jul 30, 2019

Author Contributor

Good point. I'll update that, or I'll just make the other platforms CI work. (Well, Linux still has the interior point issue...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.