Re: [PATCH] wmvcore: Add WMReader stubs.

[Alistair Leslie-Hughes]

Hi Jefferson,

On 30/03/17 10:18, Jefferson Carpenter wrote:
> I have not written unit tests for this code.  I would like to add some
> wmvcore tests, but am not very familiar with Wine's build system
> autotools, and have not been able to get tests to compile and run.

I have a basic patch for adding tests in the works, but I was waiting
for my IDL changes to be accepted first.

So, If someone wants to review the IDL changes, Since Sebastian has been
busy with other stuff.

https://source.winehq.org/patches/data/131403
> Signed-off-by: Jefferson Carpenter
>
> dlls/wmvcore/wmvcore_main.c | 1234 +++++++++++++++++++++++++++++++++++++++++++
> +static HRESULT WINAPI WMHeaderInfo_GetScriptCount(IWMHeaderInfo3 *iface, WORD *pcScripts)
> +{
> - WMReader *This = impl_from_IWMHeaderInfo3(iface);
> - FIXME("(%p)->(WMHeaderInfo_GetScriptCount)\n", This);

This is incorrect. The FIXME will insert the function name into the
logs.  You need to specify each parameter in the FIXME, there are plenty
of examples of how to do this.

Best Regards
 Alistair Leslie-Hughes

[PATCH] wmvcore: Add WMReader stubs.

[Andrew Eikum]

On Thu, Mar 30, 2017 at 08:25:03AM +0000, Alistair Leslie-Hughes wrote:
> I have a basic patch for adding tests in the works, but I was waiting
> for my IDL changes to be accepted first.
>
> So, If someone wants to review the IDL changes, Since Sebastian has been
> busy with other stuff.
>
> https://source.winehq.org/patches/data/131403

Okay :) I just noticed one thing:
> - HRESULT GetAttributeCountEx(
> -
>    [in] WORD stream_num,
> -
>    [out] WORD attributes );

If attributes is an out-param, it should be a pointer, right?

Andrew

[PATCH] wmvcore: Add WMReader stubs.

[Jefferson Carpenter]

Do FIXME statements need to print out the passed-in arguments?  Only
outputting the method name, you can still tell what needs to be
implemented.  Also, to output each argument, there are typedefs to
consider, and you have to choose whether to dereference any pointer
arguments, which makes it rather difficult to automate the process.

I could submit just the IDL changes, without the stubs added to
wmvcore_main.c.  Sorry Alistair, they seem to include your additions as
well.

On Thu, Mar 30, 2017 at 8:20 AM, Andrew Eikum 
wrote:
> On Thu, Mar 30, 2017 at 08:25:03AM +0000, Alistair Leslie-Hughes wrote:
> > I have a basic patch for adding tests in the works, but I was waiting
> > for my IDL changes to be accepted first.
> >
> > So, If someone wants to review the IDL changes, Since Sebastian has been
> > busy with other stuff.
> >
> > https://source.winehq.org/patches/data/131403
>
> Okay :) I just noticed one thing:
> > - HRESULT GetAttributeCountEx(
> > -
> >    [in] WORD stream_num,
> > -
> >    [out] WORD attributes );
>
> If attributes is an out-param, it should be a pointer, right?
>
> Andrew

[PATCH] wmvcore: Add WMReader stubs.

[Andrew Eikum]

On Thu, Mar 30, 2017 at 05:30:09PM -0500, Jefferson Carpenter wrote:
> Do FIXME statements need to print out the passed-in arguments?  Only
> outputting the method name, you can still tell what needs to be
> implemented.  Also, to output each argument, there are typedefs to
> consider, and you have to choose whether to dereference any pointer
> arguments, which makes it rather difficult to automate the process.

It's nice if you can put in the effort, but for just stubbing
interfaces I think you can get away with FIXME("%p\n", This). Actual
function implementations should trace the arguments correctly.

Andrew