Allow using Unique with interfaces by tmccombs · Pull Request #4764 · dlang/phobos

tmccombs · 2016-09-05T05:35:03Z

No description provided.

burner · 2016-09-05T21:39:27Z

The docs needs updating. They should mention interfaces

burner · 2016-09-06T08:11:34Z

std/typecons.d

 transfer can be explicit, by calling $(D release), or implicit, when
 returning Unique from a function. The resource can be a polymorphic
-class object, in which case Unique behaves polymorphically too.
+class object or instance of an interface, in which case Unique behaves polymorphically too.


shouldn't it read "or an instance"

I think the "polymorphic" here applies to both "class object" and "instance of an interface", so it's okay.

burner · 2016-09-06T08:24:55Z

I was just about to merge this than I saw https://github.com/dlang/phobos/pull/4764/files#diff-4e008aedb3026d4a84f58323e53bf017R220 (Line 220)

I think that is a better way to test the dtor call.

JackStouffer · 2017-03-16T14:36:21Z

Ping @tmccombs

PetarKirov · 2017-03-16T15:46:37Z

std/typecons.d

 {
 /** Represents a reference to $(D T). Resolves to $(D T*) if $(D T) is a value type. */
-static if (is(T:Object))
+static if (is(T == class) || is(T == interface))


Does this work with extern(C++) or extern(Windows) classes & interfaces? If not we should not allow them.

I'm not sure.

@ZombineDev Good luck detecting them. This is something that @bbasile has been working on.

I don't know about extern(Windows) - isn't that similar to extern(C) which wouldn't work with classes (I really haven't done much with Windows and D, and Windows linking is way more complicated in general than *nix)? However, as for something like extern(C++) or COM interfaces, wouldn't checking

is(T : Object) && (is(T == class) || is(T == interface))

do the trick?

That is, if the idea is to exclude them anyway.

@jmdavis the problem is that Object is a class, so if T is an interface it won't extend Object.

I don't know a lot about extern(c++) or extern(Windows), but from reading http://dlang.org/spec/interfaceToC.html, it seems like extern(Windows) isn't really relevant since it is more about function calling conventions, and from http://dlang.org/spec/cpp_interface.html it seems like this might work with C++ classes. Although it isn't really clear if it is safe to call destroy on a C++ class. Of course, Unique certainly won't clean up the memory for a C++ class, but that is true regardless of if RefT is T or T*

IMO this shouldn't be stalled on whether it works with extern(C++)/extern(Windows) classes & interfaces. If they didn't work before then this PR doesn't change the status quo.

MetaLang

LGTM. Let's get this merged.

Does anyone know what's up with vibe.d? It doesn't look related but I'd rather not merge with a failing Jenkins build.

wilzbach

IMO this shouldn't be stalled on whether it works with extern(C++)/extern(Windows) classes & interfaces. If they didn't work before then this PR doesn't change the status quo.

I agree!

wilzbach · 2017-07-21T18:55:52Z

std/typecons.d

    assert(!ub2.isEmpty);
 }

+@system unittest


FWIW: @JackStouffer has been working on getting rid of these superfluous debug statements

Good. I was going to request that they be removed but it's easy to submit a PR removing them all at once so I didn't bother.

Yep, it seems it was common style in this module

wilzbach · 2017-07-21T19:09:20Z

Does anyone know what's up with vibe.d? It doesn't look related but I'd rather not merge with a failing Jenkins build.

I restarted it - hopefully everything should be auto-merged soon :)

MetaLang · 2017-07-21T19:12:58Z

I restarted it

For future reference, how does one do that?

wilzbach · 2017-07-21T19:21:41Z

For future reference, how does one do that?

The Jenkins auto-tester now supports login via GitHub, but it's in testing stage at the moment:
dlang/ci#36

braddr · 2017-07-21T21:10:05Z

another case of an ancient pull with more than github allows # of status updates, so won't ever report as successful, but it is. I'm going to hit the merge anyway button.

wilzbach · 2017-07-21T21:17:42Z

another case of an ancient pull with more than github allows # of status updates, so won't ever report as successful, but it is. I'm going to hit the merge anyway button

Thanks for looking out! Btw do you have any ideas on preventing this? Seems like admin overwride is the only option when a PR has been built X times?

braddr · 2017-07-21T21:29:54Z

A couple thoughts, none of which work really well. Basically count the number of status update sent and after some threshold (maybe 900?):

switch to a much more conservative timing of sending status update -- only slows down the problem, but might be effective enough
change the status name (I forget what the api calls it) so that a new block of 1000 updates is available. But that doesn't work with the required setting. You could maybe use the last remaining blocks in the required main status name to do just success/failed/pending transitions, but again, only delays the consumption of the max 1000 entries.
post a final update of state pending with text that says that future updates are only available through the details link.

The real answer is that letting pull requests sit around long enough to accumulate that many updates is the problem, not that there's a limit.

Allow using Unique with interfaces

9ea653e

Add a little documentation

b6c2e38

burner reviewed Sep 6, 2016
View reviewed changes

JackStouffer added the Severity:Enhancement label Oct 26, 2016

wilzbach added the Review:Needs Work label Feb 17, 2017

wilzbach added this to the 2.075.0 milestone Mar 4, 2017

PetarKirov reviewed Mar 16, 2017

View reviewed changes

tmccombs added 3 commits March 16, 2017 12:56

Merge branch 'master' into unique-interface

894bdfe

Remove trailing space

7cbea74

Improve unit test for Unique with interface

70a9e2a

dlang-bot added the Merge:stalled label Jun 22, 2017

MartinNowak removed this from the 2.075.0 milestone Jun 25, 2017

MetaLang approved these changes Jul 21, 2017

View reviewed changes

MetaLang removed the Review:Needs Work label Jul 21, 2017

wilzbach approved these changes Jul 21, 2017

View reviewed changes

wilzbach added Merge:auto-merge and removed Merge:stalled labels Jul 21, 2017

wilzbach merged commit 4aaffed into dlang:master Jul 21, 2017

wilzbach mentioned this pull request Jul 23, 2017

Remove extra semicolon #5649

Merged

Uh oh!

Comments

Conversation

tmccombs commented Sep 5, 2016

Uh oh!

burner commented Sep 5, 2016

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!

burner commented Sep 6, 2016

Uh oh!

JackStouffer commented Mar 16, 2017

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!

tmccombs Mar 17, 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!

MetaLang left a comment • 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!

wilzbach 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!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jul 21, 2017

Uh oh!

MetaLang commented Jul 21, 2017

Uh oh!

wilzbach commented Jul 21, 2017

Uh oh!

braddr commented Jul 21, 2017

Uh oh!

wilzbach commented Jul 21, 2017

Uh oh!

braddr commented Jul 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

tmccombs Mar 17, 2017 •

edited

Loading

MetaLang left a comment •

edited

Loading