[prev in list] [next in list] [prev in thread] [next in thread] 

List:       wine-devel
Subject:    Re: [PATCH 2/6] d3drm: Implement IDirect3DRM*::CreateViewport. (v2 resend)
From:       Aaryaman Vasishta <jem456.vasishta () gmail ! com>
Date:       2016-07-27 19:39:34
Message-ID: CABVHfRutRB+EP4TAZfGTJ-HEJYW9s6bJSn3uEnzqOeXiVcKNhg () mail ! gmail ! com
[Download RAW message or body]

[Attachment #2 (multipart/alternative)]


On Thu, Jul 28, 2016 at 12:21 AM, Stefan Dösinger <stefandoesinger@gmail.com
> wrote:

> > +    if (!device || !camera)
> > +        return D3DRMERR_BADOBJECT;
> > +    if (!viewport)
> > +        return D3DRMERR_BADVALUE;
> These checks are redundant, Init() does that as well.
>
Agreed for version 3 of CreateViewport, but this this apply for version 1/2
as well? Since I'm dereferencing these interfaces there I thought the
checks were justified.

>
> > +    ok(ref4 == frame_ref, "Expected ref4 == frame_ref, got frame_ref =
> %u, ref4 = %u.\n", frame_ref, ref4);
> > +
> > +    device_ref = get_refcount((IUnknown *)device1);
> > +    frame_ref = get_refcount((IUnknown *)frame);
> The get_refcount here seems redundant because you've just shown that it is
> back to where it used to be.
>
Right, these are definitely redundant.

Thanks for the review!

Cheers,
Aaryaman

[Attachment #5 (text/html)]

<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jul \
28, 2016 at 12:21 AM, Stefan Dösinger <span dir="ltr">&lt;<a \
href="mailto:stefandoesinger@gmail.com" \
target="_blank">stefandoesinger@gmail.com</a>&gt;</span> wrote:<span \
class=""></span><br><span class=""></span><blockquote class="gmail_quote" \
style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""> \
&gt; +      if (!device || !camera)<br> &gt; +            return \
D3DRMERR_BADOBJECT;<br> &gt; +      if (!viewport)<br>
&gt; +            return D3DRMERR_BADVALUE;<br>
</span>These checks are redundant, Init() does that as \
well.<br></blockquote><div>Agreed for version 3 of CreateViewport, but this this \
apply for version 1/2 as well? Since I&#39;m dereferencing these interfaces there I \
thought the checks were justified.<br></div><blockquote class="gmail_quote" \
style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <span \
class=""><br> &gt; +      ok(ref4 == frame_ref, &quot;Expected ref4 == frame_ref, got \
frame_ref = %u, ref4 = %u.\n&quot;, frame_ref, ref4);<br> &gt; +<br>
&gt; +      device_ref = get_refcount((IUnknown *)device1);<br>
&gt; +      frame_ref = get_refcount((IUnknown *)frame);<br>
</span>The get_refcount here seems redundant because you&#39;ve just shown that it is \
back to where it used to be.<br></blockquote><div>Right, these are definitely \
redundant.<br><br></div><div>Thanks for the \
review!<br><br></div><div>Cheers,<br></div><div>Aaryaman \
<br></div></div><br></div></div>


[Attachment #6 (text/plain)]




[prev in list] [next in list] [prev in thread] [next in thread]