Revert [RetVal] annotation for multi-output DCompositionGetStatistics()
#1980
Conversation
| @@ -986,7 +986,6 @@ IWICImagingFactory::CreateDecoderFromFilename::pguidVendor=[Optional] | |||
| DCompositionCreateDevice::dcompositionDevice=[ComOutPtr] | |||
| DCompositionCreateDevice2::dcompositionDevice=[ComOutPtr] | |||
| DCompositionCreateDevice3::dcompositionDevice=[ComOutPtr] | |||
| DCompositionGetStatistics::frameStats=[RetVal] | |||
| DCompositionGetTargetStatistics::targetStats=[RetVal] | |||
There was a problem hiding this comment.
| DCompositionGetTargetStatistics::targetStats=[RetVal] |
There was a problem hiding this comment.
Disagreed. #1970 (comment).
There was a problem hiding this comment.
This attribute wasn't initially designed for functions, but if all projection owners concur, we can retain it. Typically, this type of discussion occurs before a PR for more thorough consideration beyond a single API test, but since we're already here...
@kennykerr @AArnott The proposal here is to start using the [RetVal] attribute on function parameters--not just method parameters. As a reminder, this attribute enables projections to generate code with improved ergonomics, where the HRESULT is handled internally, and the return value of the function is represented by the parameter marked with [RetVal].
There was a problem hiding this comment.
Thanks @riverar. Since we're generating "the same" syntactic sugar for functions as we are for methods, it seems to make sense to have it here.
There are only a handful of [RetVal] annotations in emitter.settings.rsp currently though.
There was a problem hiding this comment.
I'm fine with using that attribute on functions too. And it seems safe enough for projections to ignore until they decide to do the work to support it.
There was a problem hiding this comment.
On a separate note, it looks like the header has a broken name for the count field:
STDAPI DCompositionGetStatistics(
_In_ COMPOSITION_FRAME_ID frameId,
_Out_ COMPOSITION_FRAME_STATS* frameStats,
_In_ UINT targetIdCount,
_Out_writes_opt_(targetCount) COMPOSITION_TARGET_ID* targetIds,
_Out_opt_ UINT* actualTargetIdCount);s/targetCount/targetIdCount :)
There was a problem hiding this comment.
And folks keep trying to convince me SAL is used/tested internally. 😜
There was a problem hiding this comment.
Confusingly actualTargetIdCount also has to do with this :)
MarijnS95
force-pushed
the
revert-retval
branch
2 times, most recently
from
22a67bd to
2ef4c6a
Compare
…ics()` This function returns multiple values via multiple mutable pointer arguments, besides the annotated `frameStats` parameter not being the last parameter. Revert it because it has no effect on `windows-rs` and could even break in the future, because it's not intended to be used this way.
MarijnS95
force-pushed
the
revert-retval
branch
from
2ef4c6a to
00c9911
Compare
|
Seems reasonable to me if it adds value for projections that choose to honor it. |
|
@mikebattista I don't think this merged correctly or was ready for merge. Am only seeing a .txt file change? |
|
I see that too even though there was an emitter settings change as well. I'll fix it. |
For #1970 (comment)
This function returns multiple values via multiple mutable pointer arguments, besides the annotated
frameStatsparameter not being the last parameter.Revert it because it has no effect on
windows-rsand could even break in the future, because it's not intended to be used this way.