fixed issue 15721 - std.experimental.allocator dispose on interface #4022
Conversation
|
| @@ -1099,7 +1099,11 @@ void dispose(A, T)(auto ref A alloc, T p) | |||
| if (is(T == class) || is(T == interface)) | |||
| { | |||
| if (!p) return; | |||
| auto support = (cast(void*) p)[0 .. typeid(p).initializer.length]; | |||
| static if (is(T==interface)) | |||
| auto ob = cast(Object) p; | |||
There was a problem hiding this comment.
What if this cast returns null? I believe you can't cast C++ COM interfaces to Object (https://dlang.org/spec/interface.html#com-interfaces).
There was a problem hiding this comment.
I'm so used to CORBA interfaces that I forgot this crap exists.
Short answer IDK. I never use COM interface. Someone has to propose an alternative.
There was a problem hiding this comment.
Actually I think it's not a problem. COM interfaces are reference counted, so trying to free one with dispose is a programming error. So there is no problem.
There was a problem hiding this comment.
Ah, that works then. Might be worth putting in an assert that is(T: IUknown) is false.
There was a problem hiding this comment.
I'd even like to put a static assertion for this case but we need the expertise from someone who actually uses COM interfaces.
There was a problem hiding this comment.
according to https://dlang.org/spec/interface.html#com-interfaces
It cannot be the argument of a DeleteExpression.
so the static assertion to reject this case is conform.
|
#4022 updated |
|
You should be able to check whether an interface is COM or not by determining if it is a derivative of core.stdc.windows.com.IUnknown: if(is(T : IUnknown))I would say you shouldn't even compile if it's one of those (even if it's a concrete class). Only Windows should be allocating/deallocating COM classes I think. Even in the docs it says "It cannot be the argument of a DeleteExpression." Edit: Oh, right, you already did that. In my defense, I read about 3 comments in and stopped :) |
|
LGTM, just update changelog.dd |
|
As noted on #4023, changelog.dd update is not necessary, sorry for that request. Still LGTM. |
|
green again. |
|
LGTM |
|
Auto-merge toggled on |
fixed issue 15721 - std.experimental.allocator dispose on interface
ghost
deleted the
|
@bbasile mind fixing that request? I'll merge it immediately. |
|
Nevermind, I got it. |
ghost
mentioned this pull request
No description provided.