Wrap struct #2945
Wrap struct #2945
Conversation
I think ConvException could be used here. |
|
|
||
| interface Refleshable | ||
| { | ||
| int reflesh(); | ||
| } | ||
| // does not have structural conformance | ||
| static assert(!__traits(compiles, d1.wrap!Refleshable)); | ||
| static assert(!__traits(compiles, h1.wrap!Refleshable)); | ||
| // issue 12993 prevents uncommenting these asserts |
There was a problem hiding this comment.
As I understand, a compiler bug blocks this PR from not causing regressions?
There was a problem hiding this comment.
This bug is no longer reproduceable in HEAD, will look into testing with 2.067 release to close it out.
|
Ping @JesseKPhillips |
|
Apologies, I did not see this in email until today. Not that I'd do much before I moved. Thanks for the review, will look at updating. |
|
I've made the suggested changes and included new documentation for unwrap. |
|
@JakobOvrum could you remove the 'needs work' label since I've addressed the review comments. |
| @@ -3875,14 +3875,118 @@ unittest | |||
| } | |||
|
|
|||
| /** | |||
| * Determines if the $(D Source) type satisfies all interface requirements of | |||
| * $(D Targets) | |||
There was a problem hiding this comment.
Unfinished sentence. Generally this needs to be fleshed into real documentation for acceptance.
JesseKPhillips
force-pushed
the
wrapStruct
branch
from
c8cfe0b to
f57bdae
Compare
|
@andralex I've updated the doc comment and template name to implementsInterface. |
| @@ -3869,15 +3869,119 @@ unittest | |||
| assert(i.instance() is j.instance()); | |||
| } | |||
|
|
|||
| /* | |||
| * Determines if the $(D Source) type satisfies all interface requirements of | |||
| * $(D Targets). | |||
|
|
||
| Macros: | ||
|
|
||
| WIKI = Phobos/StdVariant |
There was a problem hiding this comment.
Will be pushing a fix shortly.
|
Been busy, hope to have the docs updated this weekend. |
| } | ||
|
|
||
| /* | ||
| * Avoids opCast operator overloading. |
There was a problem hiding this comment.
I'm not sure about the value of this. If someone defined a class and made it a point to override normal cast behavior, then they may have a good reason. Let them eat cake.
There was a problem hiding this comment.
This is part of the original wrap, I'm not making changes to this functionality especially with this case where I don't know what it would break.
|
Generally this lgtm, I'm a bit unsure about the handling of qualifiers. |
JesseKPhillips
force-pushed
the
wrapStruct
branch
from
a14af78 to
75d9632
Compare
|
@andralex, I've updated the docs and commented on the items I do not wish to change at this point. |
|
The documentation for this needs some work, maybe even a complete redo. For example
What does "structural conformance" mean? I don't even know where to being with the rest of the sentence because I have no idea what it's trying to say. Also, it needs examples. |
|
Ping @JesseKPhillips - As far as I can by skimming through this thread, docs are the only blocking issue? |
|
Ping @JesseKPhillips; just improve the docs and this will be ready to go. |
|
I agree that handling of qualifiers here is dubious - I'll have a stab at improving the documentation, and then possibly solving the qualifiers issue. Hopefully Jesse will have enough time to consider PRs even if he doesn't have enough time to work on this, which is completely understandable. |
|
Hm, Github doesn't seem to recognize my commit: JakobOvrum/phobos@wrapStruct...JesseKPhillips:wrapStruct Even though at a glance it looks to be structured correctly: base: https://github.com/JakobOvrum/phobos/commits/wrapStruct Well, the commit is there in my branch if anyone wants to review it, or @JesseKPhillips merge it into this PR. |
|
@JakobOvrum I've pushed up your changes to the docs. |
|
Since this is now targeting |
|
ping @JakobOvrum |
|
@quickfur Even if the docs are bad I think we can merge if it's going into stdx |
|
If @JakobOvrum doesn't respond by tomorrow I'll merge it. |
|
Auto-merge toggled on |
This implements the ability to wrap a structure into a specified interface using std.typecons.wrap. Some notes:
Bug is not an issue in HEAD.
-During these changes I came across this bug: https://issues.dlang.org/show_bug.cgi?id=12993-
When the struct is unable to be unwrapped I have thrown a ConvException. I considered using Nullable but think I was hitting issue 12993; another option would be to allow the user to specify the unwrap type to be Nullable!T.
I have pulled some logic into the isConceptWith template, which is private for now but may be something we'd want to be public and renamed?
I also added a constraint to wrap which verifies that Targets are all interfaces, which required a private template isInterface to be added.
https://issues.dlang.org/show_bug.cgi?id=14098