Re: [1/2] Added a test: registering device interface and getting devicepath

[Dmitry Timoshkov]

"Alexander Morozov" <amorozov(a)etersoft.ru> wrote:
> Changelog:
> Added a test for setupapi: registering device interface and getting device 
> path

Please consistently print GetLastError() values as %u (not %08x or %d),
otherwise the test looks good to me.


-- 
Dmitry.

[1/2] Added a test: registering device interface and getting devicepath

[Alexander Morozov]

В сообщении от Thursday 27 March 2008 16:24:52 Dmitry Timoshkov написал(а):
> "Alexander Morozov" <amorozov(a)etersoft.ru> wrote:
> > Changelog:
> > Added a test for setupapi: registering device interface and getting
> > device path
>
> Please consistently print GetLastError() values as %u (not %08x or %d),
> otherwise the test looks good to me.

Edit every test in tests/devinst.c or only testRegisterAndGetDetail?

[1/2] Added a test: registering device interface and getting devicepath

[Dmitry Timoshkov]

"Alexander Morozov" <amorozov(a)etersoft.ru> wrote:
> > Please consistently print GetLastError() values as %u (not %08x or %d),
> > otherwise the test looks good to me.
>
> Edit every test in tests/devinst.c or only testRegisterAndGetDetail?

My comment was about newly added code. If there are other places, then
they should be fixed as well, but in a separate patch.

-- 
Dmitry.

[1/2] Added a test: registering device interface and getting devicepath

[Paul Vriens]

Dmitry Timoshkov wrote:
> "Alexander Morozov" <amorozov(a)etersoft.ru> wrote:
> > > Please consistently print GetLastError() values as %u (not %08x or %d),
> > > otherwise the test looks good to me.
> > > Edit every test in tests/devinst.c or only testRegisterAndGetDetail?
>
> My comment was about newly added code. If there are other places, then
> they should be fixed as well, but in a separate patch.

Is this just for this file or do you generally want %u instead of %d/%08x ?

-- 
Cheers,

Paul.

[1/2] Added a test: registering device interface and getting devicepath

[Paul Vriens]

Paul Vriens wrote:
> Dmitry Timoshkov wrote:
> > "Alexander Morozov" <amorozov(a)etersoft.ru> wrote:
> > > > Please consistently print GetLastError() values as %u (not %08x or %d),
> > > > otherwise the test looks good to me.
> > > > Edit every test in tests/devinst.c or only testRegisterAndGetDetail?
> >
> > My comment was about newly added code. If there are other places, then
> > they should be fixed as well, but in a separate patch.
>
> Is this just for this file or do you generally want %u instead of %d/%08x ?

I'm just asking as for most of the setupapi errors %08x looks much better than 
%u (and is easier traceable in the setupapi.h include file).

For example:

#define ERROR_INVALID_MACHINENAME 
(APPLICATION_ERROR_MASK|ERROR_SEVERITY_ERROR|0x220)

%u : 3758096928
%08x : e0000220

-- 
Cheers,

Paul.

[1/2] Added a test: registering device interface and gettingdevicepath

[Dmitry Timoshkov]

"Paul Vriens" <paul.vriens.wine(a)gmail.com> wrote:
> Is this just for this file or do you generally want %u instead of %d/%08x ?

GetLastError() returns an unsigned value, and winerror.h lists error values
in decimal (if anyone really wants to match a trace with a symbolic name).

-- 
Dmitry.

[1/2] Added a test: registering device interface and gettingdevicepath

[Juan Lang]

> GetLastError() returns an unsigned value, and winerror.h lists error values
>  in decimal (if anyone really wants to match a trace with a symbolic name).

That's not strictly true.  The cryptoapi errors are defined as
HRESULTs , but are returned via GetLastError.  More relevant to this
patch is that the setupapi errors are defined as bitmasks, and are
also returned via GetLastError.

I picked %08x for a reason in this code--it's far easier to find the
relevant error using it than using %u.
--Juan

[1/2] Added a test: registering device interface andgettingdevicepath

[Dmitry Timoshkov]

"Juan Lang" <juan.lang(a)gmail.com> wrote:
> > GetLastError() returns an unsigned value, and winerror.h lists error values
> >  in decimal (if anyone really wants to match a trace with a symbolic name).
>
> That's not strictly true.  The cryptoapi errors are defined as
> HRESULTs , but are returned via GetLastError.  More relevant to this
> patch is that the setupapi errors are defined as bitmasks, and are
> also returned via GetLastError.
>
> I picked %08x for a reason in this code--it's far easier to find the
> relevant error using it than using %u.

Then the tests should consistently use %08x, not a mixture of various
formats.

-- 
Dmitry.

[1/2] Added a test: registering device interface andgettingdevicepath

[Juan Lang]

> > I picked %08x for a reason in this code--it's far easier to find the
> > relevant error using it than using %u.
>
> Then the tests should consistently use %08x, not a mixture of various
>  formats.

Perhaps.  I generally chose a format that matched the "expected"
error.  Error values like ERROR_INVALID_PARAMETER should be %u--I
erroneously use %d, I think--whereas setupapi errors should be %08x.
But since the ok output is in exceptional conditions, it might be hard
to predict which range of values is more likely, so perhaps
consistency is better than trying to be smart about it.  I don't have
a firm opinion about that.
--Juan