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

Comments

Fix Issue 18828 - [-betterC] helpless error in object.d#2178

Merged
dlang-bot merged 1 commit intodlang:masterfrom
JinShil:fix_18828
May 23, 2018
Merged

Fix Issue 18828 - [-betterC] helpless error in object.d#2178
dlang-bot merged 1 commit intodlang:masterfrom
JinShil:fix_18828

Conversation

@JinShil
Copy link
Contributor

@JinShil JinShil commented May 9, 2018

I don't really like this fix, but I'm not sure how else to do it, and maybe this PR will elicit some ideas from others.

@dlang-bot
Copy link
Contributor

dlang-bot commented May 9, 2018

Thanks for your pull request, @JinShil!

Bugzilla references

Auto-close Bugzilla Severity Description
18828 blocker [-betterC] helpless error in object.d

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

To target stable perform these two steps:

  1. Rebase your branch to upstream/stable:
git rebase --onto upstream/stable upstream/master
  1. Change the base branch of your PR to stable

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#2178"

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label May 9, 2018
@JinShil
Copy link
Contributor Author

JinShil commented May 9, 2018

I'm not sure how to test -betterC in the runtime, so, instead, I added a test at dlang/dmd#8235

@JinShil JinShil added the Vision Vision Plan https://wiki.dlang.org/Vision/2018H1 label May 19, 2018
RazvanN7
RazvanN7 approved these changes May 23, 2018
@jacob-carlborg
Copy link
Contributor

Is this testable?

@JinShil
Copy link
Contributor Author

JinShil commented May 23, 2018

Is this testable?

Yes, at dlang/dmd#8235. I didn't know how else to do it.

@jacob-carlborg
Copy link
Contributor

Yes, at dlang/dmd#8235. I didn't know how else to do it.

Is this related to how the tests are setup or run in druntime? Because I don't see anything special in that test.

@JinShil
Copy link
Contributor Author

JinShil commented May 23, 2018

Notice this at the top of the test file:

 /* REQUIRED_ARGS: -betterC

This means the test will be compiled in -betterC mode. In -betterC you cannot use TypeInfo, so using typeid will cause the compiler to emit an error. The current implementation of destroy (prior to this PR) uses typeid, which is invalid in -betterC.

You can see a demonstration of the test currently failing at https://run.dlang.io/is/vPcZNA. Once this is pulled the DMD test should pass.

@jacob-carlborg
Copy link
Contributor

Not sure about how much work it would be, but what about adding a new directory here: https://github.com/dlang/druntime/tree/master/test ?

@JinShil JinShil requested a review from wilzbach as a code owner May 23, 2018 09:47
@JinShil
Copy link
Contributor Author

JinShil commented May 23, 2018

I added the directory and test to the best of my ability, but I don't even understand what most of the syntax in the Makefile is for and I'm not even sure if the test is running.

EDIT: See comment below.

@jacob-carlborg
Copy link
Contributor

I added the directory and test to the best of my ability, but I don't even understand what most of the syntax in the Makefile is for and I'm not even sure if the test is running.

Add assert(false); to verify that the tests are running and failing.

@JinShil
Copy link
Contributor Author

JinShil commented May 23, 2018

It appears to be running now. I had to add the test to the root posix.mak file and output appeared (e.g. https://auto-tester.puremagic.com/show-run.ghtml?projectid=1&runid=3176138&dataid=22521943&isPull=true search for "betterc").

It appears none of the tests in the test folder are run for Windows except uuid. I think that's ok for this test as there's nothing platform specific about it.

@RazvanN7
Copy link
Contributor

Can we close : dlang/dmd#8235 now ?

@dlang-bot dlang-bot merged commit 9ade20c into dlang:master May 23, 2018
@schveiguy
Copy link
Member

Haha, I just went into bugzilla to make a new issue about the inefficiency of destroy, to find that the code I swore was using typeid is no longer there. And lo and behold, it's changed, but for another reason.

I'm still going to file an issue, because I feel like smaller structs can be done much more efficiently than casting to ubyte arrays. But this change helps significantly.

See: https://forum.dlang.org/post/pe3pcp$13t4$1@digitalmars.com

@schveiguy
Copy link
Member

I'm still going to file an issue

See https://issues.dlang.org/show_bug.cgi?id=18899

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

Labels

Bug Fix Include reference to corresponding bugzilla issue Vision Vision Plan https://wiki.dlang.org/Vision/2018H1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants