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

std.typecons.RefCounted!(T, RefCountedAutoInitialize.no) should still work when T.this() is annotated with @disable #7797

Merged
merged 1 commit into from
Feb 22, 2021

Conversation

n8sh
Copy link
Member

@n8sh n8sh commented Feb 15, 2021

No description provided.

@n8sh n8sh requested a review from MetaLang as a code owner February 15, 2021 19:54
@dlang-bot
Copy link
Contributor

dlang-bot commented Feb 15, 2021

Thanks for your pull request, @n8sh!

Bugzilla references

Auto-close Bugzilla Severity Description
21638 enhancement std.typecons.RefCounted!(T, RefCountedAutoInitialize.no) should still work when T.this() is annotated with @disable

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#7797"

std/typecons.d Outdated
Comment on lines 6489 to 6495
void ensureInitialized()
void ensureInitialized()()
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if making this a zero-args template is the cleanest solution for user-facing code. Would it be better to use a static if?

@n8sh n8sh force-pushed the issue-21638 branch 2 times, most recently from cc75ab9 to dafc68d Compare February 16, 2021 02:39
@n8sh n8sh changed the title std.typecons.Refcounted!(T, RefCountedAutoInitialize.no) should still work when T.this() is annotated with @disable std.typecons.RefCounted!(T, RefCountedAutoInitialize.no) should still work when T.this() is annotated with @disable Feb 16, 2021
@RazvanN7
Copy link
Collaborator

Can you please provide some information on why templating the ensureInitialized method fixes this issue?

@n8sh
Copy link
Member Author

n8sh commented Feb 16, 2021

@RazvanN7 templated code isn't compiled unless instantiated. ensureInitialized in turn instantiates initialize with zero arguments which fails to compile if T.this() is annotated with @disable. If the reason ensureInitialized is now a template isn't clear to someone who is very familiar with D programming that increases my doubt as to whether it's a good approach.

@RazvanN7
Copy link
Collaborator

What I meant is that you can still call ensureInitialized directly and that will result in a compilation error even if it is templated. The proper fix should make sure that ensureInitialized exists only when AutoInitialize = yes and error out ensureInitialized is used otherwise.

@n8sh
Copy link
Member Author

n8sh commented Feb 16, 2021

There's no reason someone couldn't have been calling ensureInitialized manually with RefCountedAutoInitialize.no. It's a documented public method. So I would only remove it when it actually wouldn't compile.

Is there a better way to check for a @disable this() than !__traits(compiles, { static T t; } )?

@RazvanN7
Copy link
Collaborator

There's no reason someone couldn't have been calling ensureInitialized manually with RefCountedAutoInitialize.no

Right.

So I would only remove it when it actually wouldn't compile.

Actually, now that you mention, ideally, the method would always be present but it would assert when used
with a struct that has a disabled default constructor.

Is there a better way to check for a @disable this() than !__traits(compiles, { static T t; } )?

Not that I can think of.

@n8sh
Copy link
Member Author

n8sh commented Feb 16, 2021

Actually, now that you mention, ideally, the method would always be present but it would assert when used
with a struct that has a disabled default constructor.

You mean at runtime rather than compile time?

@RazvanN7
Copy link
Collaborator

You mean at runtime rather than compile time?

Yes

@n8sh
Copy link
Member Author

n8sh commented Feb 16, 2021

That would make sense if ensureInitialized is being conceptualized as an extra-helpful enforceInitialized. "The RefCounted should have already been initialized, so if by some oversight it hasn't been we will try to fix the situation if we know how."

But ensureInitialized isn't necessarily a fallback. It can be intentionally used as the path by which a RefCounted is initialized. In such cases it would be a huge step backwards to let the code compile but crash when executed.

@RazvanN7
Copy link
Collaborator

You have a point. However, the compilation error needs to be more targeted. IMHO it's a problem if you call ensure initialized but you get some error in the innards of std.conv. Basically, we need to spell out to the user that it's not possible to use ensureInitialized if T has disabled default construction.

…e.no) should still work when T.this() is annotated with `@disable`
@n8sh
Copy link
Member Author

n8sh commented Feb 17, 2021

Added a static assert with an informative error message.

@RazvanN7 RazvanN7 merged commit b74f782 into dlang:master Feb 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants