Make std.typecons.Tuple betterC compatible by wilzbach · Pull Request #5952 · dlang/phobos

wilzbach · 2017-12-21T13:11:57Z

I'm not sure how people want to start testing -betterC compatibility with Phobos. I assume the best way would be to do extract all unittests marked with @betterC?

unittest @betterC
{
....
}

For now I went with a more simple and straight-forward way of simply executing all files in test/betterC.


Also I need to define `main` manually as the default main with `void main` returns with `44` :/

dlang-bot · 2017-12-21T13:11:59Z

Thanks for your pull request, @wilzbach!

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 + phobos#5952"

PetarKirov · 2017-12-22T10:34:48Z

posix.mak

+test/betterC/%.run: test/betterC/%.d $(DMD) $(LIB)
+	mkdir -p $(ROOT)/unittest/betterC
+	$(DMD) $(DFLAGS) -of$(ROOT)/unittest/betterC/$(notdir $(basename $<)) -betterC $(UDFLAGS) $(LIB) \
+		-defaultlib= -debuglib= $(LINKDL) $<


-betterC implies that we're not linking druntime and phobos -> drop $(LIB). This should hopefully get rid of the current linker error, though we get a new one.

Hehe I thought so too and I tried this in the beginning, but then I get more linker errors:

generated/linux/release/64/unittest/betterC/test18101.o:test/betterC/test18101.d:function _D3std8typecons__T5TupleTiVAyaa6_736368656d61Z17injectNamedFieldsFZQBo: error: undefined reference to '_D12TypeInfo_Aya6__initZ' generated/linux/release/64/unittest/betterC/test18101.o:test/betterC/test18101.d:function _D3std8typecons__T5TupleTiVAyaa6_736368656d61Z17injectNamedFieldsFZQBo: error: undefined reference to '_d_arraycatnTX' generated/linux/release/64/unittest/betterC/test18101.o:test/betterC/test18101.d:function _D3std8typecons__T5TupleTiVAyaa6_736368656d61Z17injectNamedFieldsFZQBo: error: undefined reference to '_D12TypeInfo_Aya6__initZ' generated/linux/release/64/unittest/betterC/test18101.o:test/betterC/test18101.d:function _D3std8typecons__T5TupleTiVAyaa6_736368656d61Z17injectNamedFieldsFZQBo: error: undefined reference to '_d_arrayappendT' generated/linux/release/64/unittest/betterC/test18101.o:test/betterC/test18101.d:function _D3std8typecons__T5TupleTiVAyaa6_736368656d61Z17injectNamedFieldsFZQBo: error: undefined reference to '_D12TypeInfo_Aya6__initZ' generated/linux/release/64/unittest/betterC/test18101.o:test/betterC/test18101.d:function _D3std8typecons__T5TupleTiVAyaa6_736368656d61Z17injectNamedFieldsFZQBo: error: undefined reference to '_d_arraycatnTX' generated/linux/release/64/unittest/betterC/test18101.o:test/betterC/test18101.d:function _D3std8typecons__T5TupleTiVAyaa6_736368656d61Z17injectNamedFieldsFZQBo: error: undefined reference to '_D12TypeInfo_Aya6__initZ' generated/linux/release/64/unittest/betterC/test18101.o:test/betterC/test18101.d:function _D3std8typecons__T5TupleTiVAyaa6_736368656d61Z17injectNamedFieldsFZQBo: error: undefined reference to '_d_arrayappendT' generated/linux/release/64/unittest/betterC/test18101.o:test/betterC/test18101.d:function _D3std8typecons__T5TupleTiZ17injectNamedFieldsFZAya: error: undefined reference to '_d_arraycatnTX' generated/linux/release/64/unittest/betterC/test18101.o:test/betterC/test18101.d:function _D3std8typecons__T5TupleTiZ17injectNamedFieldsFZAya: error: undefined reference to '_d_arrayappendT' generated/linux/release/64/unittest/betterC/test18101.o:test/betterC/test18101.d:function _D3std8typecons__T5TupleTiVAyaa6_736368656d61ZQBc6toHashMxFNbNeZm: error: undefined reference to '_D10TypeInfo_i6__initZ' generated/linux/release/64/unittest/betterC/test18101.o:test/betterC/test18101.d:function _D3std8typecons__T5TupleTiZQj6toHashMxFNbNeZm: error: undefined reference to '_D10TypeInfo_i6__initZ' collect2: error: ld returned 1 exit status

There are two culprits here:

injectNamedFields gets codegened -> we need to make sure that it exist only in CTFE context. One way to do this is to implement it as enum injectNamedFields = { /* code */ }();

toHash uses typeid(T).getHash -> try replacing this with some template function which will get codegen-ed on demand (so that we don't need to linking druntime). You can try using core.internal.hash : hashOf, or if that doesn't work, we can workaround it temporary with:

version (D_BetterC) static assert (0, "Not implemented"); else: /* same as before ... */

and making Tuple.toHash a parameter-less template.

Worked like a charm. Thanks!

TypeInfo is part of DRuntime. See: dlang#5952

TypeInfo is part of DRuntime. See: dlang#5952 Replaced `toHash` with a parameter-less template for codegen on demand.

wilzbach · 2017-12-22T12:22:09Z

std/typecons.d

                A `size_t` representing the hash of this `Tuple`.
         */
-        size_t toHash() const nothrow @trusted
+        size_t toHash()() const nothrow @trusted


Looks like why can't change this to a template:

std/functional.d(1003): Error: AA key type Tuple should have 'size_t toHash() const nothrow @safe' if opEquals defined generated/linux/debug/64/publictests/std_functional.d(193): Error: template instance std_functional.__unittest_generated_linux_debug_64_publictests_std_functional_d_187_15.memoize!(fib) error instantiating std/functional.d(1003): Error: AA key type Tuple should have 'size_t toHash() const nothrow @safe' if opEquals defined generated/linux/debug/64/publictests/std_functional.d(205): Error: template instance std_functional.__unittest_generated_linux_debug_64_publictests_std_functional_d_199_16.memoize!(fact) error instantiating std/functional.d(1003): Error: AA key type Tuple should have 'size_t toHash() const nothrow @safe' if opEquals defined generated/linux/debug/64/publictests/std_functional.d(219): Error: template instance std_functional.__unittest_generated_linux_debug_64_publictests_std_functional_d_211_17.memoize!(factImpl) error instantiating

How about simply removing the method for betterC for now?

How about this:

size_t toHash() const nothrow @trusted { version(D_BetterC) { // Disabled in -betterC for now as TypeInfo isn't present assert(0, "Not implemented"); } else { size_t h = 0; foreach (i, T; Types) h += typeid(T).getHash(cast(const void*)&field[i]); return h; } }

That would fail at runtime which is strictly worse then compile-time failure.

Of course, that's why my first suggestion was to make toHash a template function and use static assert. Thinking about it more I don't see why we define our own opEquals and toHash, instead of relying on the compiler generated ones.

TypeInfo is part of DRuntime. See: dlang#5952 Replaced `toHash` with a parameter-less template for codegen on demand.

wilzbach · 2018-07-19T14:49:07Z

Finally got around rebasing this. It looks like we can get around the problems of the static assert errors if we use the user-facing hashOf implementation.

PetarKirov

I'm glad to see that this finally working!

wilzbach · 2018-07-19T15:24:57Z

std/typecons.d

            static foreach (i, T; Types)
            {{
-                const k = typeid(T).getHash((() @trusted => cast(const void*) &field[i])());
+                const k = hashOf(field[i]);


Hmm, looks like hashOf can't handle void[] arrays:

/var/lib/jenkins/dlang_projects@4/distribution/bin/../imports/core/internal/hash.d(218,30): Error: template instance `core.internal.hash.hashOf!(const(void[32]))` error instantiating /var/lib/jenkins/dlang_projects@4/distribution/bin/../imports/std/typecons.d(1177,33): instantiated from here: `hashOf!(const(Json))` /var/lib/jenkins/dlang_projects@4/distribution/bin/../imports/std/typecons.d(640,23): instantiated from here: `Tuple!(string, Json)`

@n8sh as you have worked a lot on hashOf, maybe you have a good idea on how to fix this?
Can we cast void arrays to ubyte for hashing?

@wilzbach hashOf void arrays is currently working in druntime master.

wilzbach · 2018-07-20T16:36:28Z

Opened a separate PR that just adds the testsuite (#6640), s.t. we can already add the betterC testsuite.

TypeInfo is part of DRuntime. See: dlang#5952 Replaced `toHash` with a parameter-less template for codegen on demand.

wilzbach · 2018-08-14T13:11:35Z

Well, we can't use Tuple!(int, Json) yet, but thanks to a few recent changes in druntime this seems to be finally passing.
I added a very simplistic -betterC test in the test/betterc testsuite, but once we finally get @betterC I plan on annotating a few unittests, s.t. we can ensure Tuple will continue to work in -betterC.

PetarKirov · 2018-08-14T14:27:55Z

test/betterC/typecons.d

@@ -0,0 +1,7 @@
+extern(C) void main()


Can you now move it inside std.typecons as a @BetterC unittest?

I can/will do this once #6645 has been merged.

n8sh · 2018-08-16T16:25:08Z

Unfortunately, core.internal.hashOf is not wholly betterC compatible because of hashOf's need to produce bitwise representations of structs in CTFE. The various core.internal.convert.toUbyte overloads have allocation with new in their CTFE-only paths.

PetarKirov · 2018-08-19T08:26:17Z

@n8sh the enum function trick used in this PR may help for hashOf's use of CTFE. (Though of course the compiler should also be fixed.)

n8sh · 2018-08-19T16:15:36Z

std/typecons.d

                A `size_t` representing the hash of this `Tuple`.
         */
-        size_t toHash() const nothrow @safe
+        size_t toHash() const nothrow @trusted


This can't be legitimately marked @trusted because hashOf(x) can call x.toHash() which may do any number of unsafe things. It also can't be @trusted because of the "Object.toHash() is not const" fiasco.

n8sh · 2018-08-19T18:41:45Z

@ZombineDev I don't think it can be used there: you can't treat function arguments as compile-time constants even in an if (__ctfe) block. Or did you mean something else?

TypeInfo is part of DRuntime. See: dlang#5952 Replaced `toHash` with a parameter-less template for codegen on demand.

wilzbach · 2018-12-31T12:14:08Z

Rebased and removed the @trusted. Looks like it's no longer required (:

CC @thewilsonator

wilzbach added the Review:WIP Work In Progress - not ready for review or pulling label Dec 21, 2017

wilzbach requested review from CyberShadow and MartinNowak as code owners December 21, 2017 13:11

dlang-bot added Severity:Bug Fix Severity:Enhancement labels Dec 21, 2017

PetarKirov reviewed Dec 22, 2017

View reviewed changes

wilzbach force-pushed the betterC branch from 76c42a0 to 24db3a7 Compare December 22, 2017 11:34

wilzbach requested a review from MetaLang as a code owner December 22, 2017 11:34

wilzbach added a commit to wilzbach/phobos that referenced this pull request Dec 22, 2017

Disabled toHash in -betterC for now

24db3a7

TypeInfo is part of DRuntime. See: dlang#5952

wilzbach force-pushed the betterC branch from 24db3a7 to 67f1edc Compare December 22, 2017 11:38

wilzbach added a commit to wilzbach/phobos that referenced this pull request Dec 22, 2017

Disabled toHash in -betterC for now

67f1edc

TypeInfo is part of DRuntime. See: dlang#5952 Replaced `toHash` with a parameter-less template for codegen on demand.

wilzbach removed the Review:WIP Work In Progress - not ready for review or pulling label Dec 22, 2017

wilzbach force-pushed the betterC branch from 67f1edc to 598b24c Compare December 22, 2017 12:04

wilzbach added a commit to wilzbach/phobos that referenced this pull request Dec 22, 2017

Disabled toHash in -betterC for now

598b24c

TypeInfo is part of DRuntime. See: dlang#5952 Replaced `toHash` with a parameter-less template for codegen on demand.

wilzbach commented Dec 22, 2017

View reviewed changes

dlang-bot added Review:Needs Work Merge:Needs Rebase and removed Merge:Needs Rebase labels Dec 29, 2017

wilzbach force-pushed the betterC branch from 598b24c to afe5406 Compare January 19, 2018 03:20

wilzbach added a commit to wilzbach/phobos that referenced this pull request Jan 19, 2018

Disabled toHash in -betterC for now

afe5406

TypeInfo is part of DRuntime. See: dlang#5952 Replaced `toHash` with a parameter-less template for codegen on demand.

dlang-bot removed the Review:Needs Work label Jan 19, 2018

This was referenced Jan 19, 2018

Start to move away from complex and imaginary types #6014

Merged

Deprecate imaginary and complex types dlang/dmd#7640

Closed

wilzbach force-pushed the betterC branch from afe5406 to 91a69a6 Compare July 19, 2018 14:38

wilzbach added a commit to wilzbach/phobos that referenced this pull request Jul 19, 2018

Disabled toHash in -betterC for now

91a69a6

TypeInfo is part of DRuntime. See: dlang#5952 Replaced `toHash` with a parameter-less template for codegen on demand.

wilzbach force-pushed the betterC branch from 91a69a6 to 60924b6 Compare July 19, 2018 14:43

wilzbach added a commit to wilzbach/phobos that referenced this pull request Jul 19, 2018

Disabled toHash in -betterC for now

60924b6

TypeInfo is part of DRuntime. See: dlang#5952 Replaced `toHash` with a parameter-less template for codegen on demand.

wilzbach force-pushed the betterC branch from 60924b6 to b5970ce Compare July 19, 2018 14:47

wilzbach added a commit to wilzbach/phobos that referenced this pull request Jul 19, 2018

Disabled toHash in -betterC for now

b5970ce

TypeInfo is part of DRuntime. See: dlang#5952 Replaced `toHash` with a parameter-less template for codegen on demand.

wilzbach changed the title ~~[RFC] Add -betterC testsuite for Phobos~~ Add -betterC testsuite for Phobos Jul 19, 2018

PetarKirov approved these changes Jul 19, 2018

View reviewed changes

wilzbach commented Jul 19, 2018

View reviewed changes

wilzbach mentioned this pull request Jul 20, 2018

Add -betterC testsuite for Phobos #6640

Merged

wilzbach force-pushed the betterC branch from b5970ce to ca0effb Compare August 14, 2018 13:01

wilzbach added a commit to wilzbach/phobos that referenced this pull request Aug 14, 2018

Disabled toHash in -betterC for now

ca0effb

TypeInfo is part of DRuntime. See: dlang#5952 Replaced `toHash` with a parameter-less template for codegen on demand.

wilzbach changed the title ~~Add -betterC testsuite for Phobos~~ Make std.typecons.Tuple betterC compatible Aug 14, 2018

PetarKirov reviewed Aug 14, 2018

View reviewed changes

n8sh reviewed Aug 19, 2018

View reviewed changes

Avoid injectNamedFields to be codegened

c4c7293

wilzbach force-pushed the betterC branch from dfcf8c6 to dead392 Compare December 31, 2018 11:58

wilzbach added a commit to wilzbach/phobos that referenced this pull request Dec 31, 2018

Disabled toHash in -betterC for now

08bc75a

TypeInfo is part of DRuntime. See: dlang#5952 Replaced `toHash` with a parameter-less template for codegen on demand.

wilzbach force-pushed the betterC branch from dead392 to 13705f5 Compare December 31, 2018 12:02

Add a -betterC test for std.typecons.Tuple

13705f5

thewilsonator approved these changes Dec 31, 2018

View reviewed changes

n8sh added the betterC label Dec 31, 2018

thewilsonator added the Merge:auto-merge label Dec 31, 2018

dlang-bot merged commit 8e17885 into dlang:master Dec 31, 2018

PetarKirov mentioned this pull request Jul 29, 2019

Make TempStringNoAlloc a template dlang/druntime#2705

Merged

PetarKirov mentioned this pull request Apr 5, 2020

Evaluate Pure Functions With CTFE dlang/DIPs#177

Open

Uh oh!

Conversation

wilzbach commented Dec 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Dec 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bugzilla references

Testing this PR locally

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PetarKirov Dec 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jul 19, 2018

Uh oh!

PetarKirov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jul 20, 2018

Uh oh!

wilzbach commented Aug 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n8sh commented Aug 16, 2018

Uh oh!

PetarKirov commented Aug 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n8sh commented Aug 19, 2018

Uh oh!

wilzbach commented Dec 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wilzbach commented Dec 21, 2017 •

edited

Loading

dlang-bot commented Dec 21, 2017 •

edited

Loading

PetarKirov Dec 22, 2017 •

edited

Loading