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 18493 - [betterC] Can't use aggregated type with postblit#2184

Merged
dlang-bot merged 1 commit intodlang:masterfrom
JinShil:betterC_nothrow
Jun 12, 2018
Merged

Fix Issue 18493 - [betterC] Can't use aggregated type with postblit#2184
dlang-bot merged 1 commit intodlang:masterfrom
JinShil:betterC_nothrow

Conversation

@JinShil
Copy link
Contributor

@JinShil JinShil commented May 16, 2018

This PR is required for dlang/dmd#8253. Please read the motivation there for context.

Despite the enabling features this provides for dlang/dmd#8253, this PR is still quite useful and forward-looking.

Although -betterC does not link with druntime or phobos, it still imports object.d. There is a large amount of code in object.d that not utilized when compiling with -betterC, but it is not versioned out like it should be, and that causes the compiler to emit errors for code that isn't even being utilized. This is not only necessary for dlang/dmd#8253, but also for other planned -betterC features like enabling the use of classes with only static members (See dlang/dmd#8204 for an already merged PR enabling this for minimal runtime builds).

It is likely future PRs will need to be made to hoist certain templates out of the version(Not_BetterC) area up to the version(D_BetterC)` area as users utilize -betterC more and file issues identifying certain language features that don't work in a -betterC environment. It will also be beneficial to acquire those issue reports to identify lapses in test coverage for -betterC.

EDIT: #2194 took care of the excessive importing. This PR is now only needed to support dlang/dmd#8253.

@dlang-bot
Copy link
Contributor

dlang-bot commented May 16, 2018

Thanks for your pull request, @JinShil!

Bugzilla references

Auto-close Bugzilla Severity Description
18493 blocker [betterC] Can't use aggregated type with postblit

⚠️⚠️⚠️ 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#2184"

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label May 16, 2018
src/object.d Outdated
{
extern (C) Object _d_newclass(const TypeInfo_Class ci);
extern (C) void rt_finalize(void *data, bool det=true);
nothrow:
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't work (attributes in a version block don't apply to the outer scope).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. What's the proper idiom, then?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don think that there is one. I tried something along the lines of :

import std.stdio: writeln;

version(V1)
{
    enum versionl = true;
}
else
{
}

static if (versionl)
{
    notrow:
}

void f() 
{
    writeln("bla");
}

But it doesn't work since it looks like nothrow: cannot be placed inside a static if body. If static if doesn't work and nothrow: inside a version body does not work for the outer scope I see no other solution then to actually declare the code as a string and mixin it in each branch. I hope that someone has a better idea, cause the one I proposed is awful.

Copy link
Member

Choose a reason for hiding this comment

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

You could put the optionally nothrow code into a template mixin, and mixin the template on each leaf of the version statement.

Copy link
Contributor Author

@JinShil JinShil May 28, 2018

Choose a reason for hiding this comment

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

You could put the optionally nothrow code into a template mixin, and mixin the template on each leaf of the version statement.

Oh, man, that would be great if it worked. Please illustrate with an example, as I don't quite follow. You can see my prior attempts here: https://forum.dlang.org/post/uhgzgmowqcczczrdtsol@forum.dlang.org

EDIT: scratch that. You're right. It works. Awesome.

@JinShil JinShil added the Vision Vision Plan https://wiki.dlang.org/Vision/2018H1 label May 19, 2018
@JinShil JinShil added WIP Work In Progress - not ready for review or pulling and removed auto-merge labels May 28, 2018
@JinShil
Copy link
Contributor Author

JinShil commented May 28, 2018

I Removed the auto-merge label. This needs a solution to the nothrow: attribution problem discussed above. Also, @WalterBright did not approve of the corresponding DMD PR.

@WalterBright If you'd like, I can create a new PR to prevent importing unneeded code in object.d when compiling with -betterC. I think that should be a separate PR to prevent conflating it with the nothrow issue.

@JinShil
Copy link
Contributor Author

JinShil commented May 29, 2018

@WalterBright, I'm sorry for interrupting the workflow. I got a little confused since you objected to the DMD PR, but approved this. I see now that this PR has value regardless of the fate of the DMD PR.

I also implemented your elegant solution to the conditional attribute problem. It turns out, however, it wasn't needed because the templated code has its attributes inferred by the caller. Nevertheless, since this idiom was hard-won, I left the boiler-plate in place so, if we find code that needs this conditional attribution, we know where to put it. I predict this code will need to be added to and enhanced as we continue to implement -betterC.

Please give it another look. Once this is merged, I should be able to address https://issues.dlang.org/show_bug.cgi?id=18905 with more predicatable results.

@JinShil
Copy link
Contributor Author

JinShil commented May 29, 2018

#2194 Should be merged first. It has the same changes as this PR, but removes the controversy around the nothrow issue. After that is merged, the fate of this PR can be decided in isolation based on the merits of the nothrow semantics of -betterC.

@JinShil JinShil closed this Jun 1, 2018
@JinShil JinShil deleted the betterC_nothrow branch June 1, 2018 22:14
@JinShil JinShil restored the betterC_nothrow branch June 1, 2018 22:44
@JinShil JinShil reopened this Jun 1, 2018
@JinShil JinShil removed the Blocked label Jun 1, 2018
@JinShil JinShil changed the title Fix Issue 18493 - [betterC] Can't use aggregated type with postblit (+More) Fix Issue 18493 - [betterC] Can't use aggregated type with postblit Jun 1, 2018
@JinShil
Copy link
Contributor Author

JinShil commented Jun 1, 2018

#2194 took care of the excessive importing. This PR is now only needed to support dlang/dmd#8253.

@JinShil
Copy link
Contributor Author

JinShil commented Jun 1, 2018

@WalterBright I'm seeing some strange undefined reference errors in the Circle CI tests. I think they're related to backend conversion to D. Any ideas?

@rainers
Copy link
Member

rainers commented Jun 2, 2018

I'm seem some strange undefined reference errors in the Circle CI tests. I think they're related to backend conversion to D. Any ideas?

You'll need to upgrade the D host compiler (or remove -betterC from the command line for dvec.d).

@JinShil
Copy link
Contributor Author

JinShil commented Jun 3, 2018

You'll need to upgrade the D host compiler

Done. #2205

@JinShil
Copy link
Contributor Author

JinShil commented Jun 12, 2018

Ping! Why hasn't this merged?

@dlang-bot dlang-bot merged commit a2cabb8 into dlang:master Jun 12, 2018
@JinShil JinShil deleted the betterC_nothrow branch June 12, 2018 07:36
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.

7 participants