Re: winhttp/tests: Use BOOL type where appropriate

[Dmitry Timoshkov]

Frédéric Delanoy  wrote:
> --- a/dlls/winhttp/tests/notification.c
> +++ b/dlls/winhttp/tests/notification.c
> @@ -44,9 +44,9 @@ struct notification
>  {
>      enum api function;      /* api responsible for notification */
>      unsigned int status;    /* status received */
> - int todo;
> - int ignore;
> - int skipped_for_proxy;
> - BOOL todo;
> - BOOL ignore;
> - BOOL skipped_for_proxy;

Do you realize that BOOL is typedef'ed to int and therefore your patches
do basically nothing? Is there any real reason behind these patches?

winhttp/tests: Use BOOL type where appropriate

[Bruno Jesus]

On Wed, Nov 27, 2013 at 6:16 AM, Dmitry Timoshkov  wrote:
> Frédéric Delanoy  wrote:
> > --- a/dlls/winhttp/tests/notification.c
> > +++ b/dlls/winhttp/tests/notification.c
> > @@ -44,9 +44,9 @@ struct notification
> >  {
> >      enum api function;      /* api responsible for notification */
> >      unsigned int status;    /* status received */
> > - int todo;
> > - int ignore;
> > - int skipped_for_proxy;
> > - BOOL todo;
> > - BOOL ignore;
> > - BOOL skipped_for_proxy;
>
> Do you realize that BOOL is typedef'ed to int and therefore your patches
> do basically nothing? Is there any real reason behind these patches?

Hi, Dmitry. This has been discussed before, here is an example:
http://osdir.com/ml/wine-devel/2013-10/msg00131.html
> --
> Dmitry.

Best wishes,
Bruno

winhttp/tests: Use BOOL type where appropriate

[Dmitry Timoshkov]

Bruno Jesus  wrote:
> > > --- a/dlls/winhttp/tests/notification.c
> > > +++ b/dlls/winhttp/tests/notification.c
> > > @@ -44,9 +44,9 @@ struct notification
> > >  {
> > >      enum api function;      /* api responsible for notification */
> > >      unsigned int status;    /* status received */
> > > - int todo;
> > > - int ignore;
> > > - int skipped_for_proxy;
> > > - BOOL todo;
> > > - BOOL ignore;
> > > - BOOL skipped_for_proxy;
> >
> > Do you realize that BOOL is typedef'ed to int and therefore your patches
> > do basically nothing? Is there any real reason behind these patches?
>
> Hi, Dmitry. This has been discussed before, here is an example:
> http://osdir.com/ml/wine-devel/2013-10/msg00131.html

I don't think that qualifies as a "discussion" :) Besides the using
Windows-only BOOL type these patches break the git history and don't
add any advertized clarity IMHO.

winhttp/tests: Use BOOL type where appropriate

[Michael Stefaniuc]

On 11/27/2013 12:52 PM, Dmitry Timoshkov wrote:
> Bruno Jesus  wrote:
> > > > --- a/dlls/winhttp/tests/notification.c
> > > > +++ b/dlls/winhttp/tests/notification.c
> > > > @@ -44,9 +44,9 @@ struct notification
> > > >  {
> > > >      enum api function;      /* api responsible for notification */
> > > >      unsigned int status;    /* status received */
> > > > - int todo;
> > > > - int ignore;
> > > > - int skipped_for_proxy;
> > > > - BOOL todo;
> > > > - BOOL ignore;
> > > > - BOOL skipped_for_proxy;
> > >
> > > Do you realize that BOOL is typedef'ed to int and therefore your patches
> > > do basically nothing? Is there any real reason behind these patches?
> >
> > Hi, Dmitry. This has been discussed before, here is an example:
> > http://osdir.com/ml/wine-devel/2013-10/msg00131.html
>
> I don't think that qualifies as a "discussion" :) Besides the using
> Windows-only BOOL type these patches break the git history and don't
> add any advertized clarity IMHO.

Again this is an optimization for the human reader. BOOL, TRUE and FALSE
make it easier to read the code without needing too much context.

But it isn't restricted to that. Other tools can do more strict checks
like warning about questionable opertations like arithmetic operations
with BOOLs. Even before Frederic started I've stumbled over Wine code
doing exactly that (oleaut32 in VarFormatNumber()) but the respective
code is "ugly" so I couldn't force myself to rewrite it and get rid of
the issue.

bye
    michael

winhttp/tests: Use BOOL type where appropriate

[Dmitry Timoshkov]

Michael Stefaniuc  wrote:
> > > > > --- a/dlls/winhttp/tests/notification.c
> > > > > +++ b/dlls/winhttp/tests/notification.c
> > > > > @@ -44,9 +44,9 @@ struct notification
> > > > >  {
> > > > >      enum api function;      /* api responsible for notification */
> > > > >      unsigned int status;    /* status received */
> > > > > - int todo;
> > > > > - int ignore;
> > > > > - int skipped_for_proxy;
> > > > > - BOOL todo;
> > > > > - BOOL ignore;
> > > > > - BOOL skipped_for_proxy;
> > > >
> > > > Do you realize that BOOL is typedef'ed to int and therefore your patches
> > > > do basically nothing? Is there any real reason behind these patches?
> > >
> > > Hi, Dmitry. This has been discussed before, here is an example:
> > > http://osdir.com/ml/wine-devel/2013-10/msg00131.html
> >
> > I don't think that qualifies as a "discussion" :) Besides the using
> > Windows-only BOOL type these patches break the git history and don't
> > add any advertized clarity IMHO.
>
> Again this is an optimization for the human reader. BOOL, TRUE and FALSE
> make it easier to read the code without needing too much context.

Is breaking git history really justified with this kind of patches?
Isn't this from the same (or close enough) area as changing hungarian
notation or replacing LP(C)STR by (const) char * on the whole Wine code
base?

winhttp/tests: Use BOOL type where appropriate

[Michael Stefaniuc]

On 11/27/2013 01:43 PM, Dmitry Timoshkov wrote:
> Michael Stefaniuc  wrote:
> > > > > > --- a/dlls/winhttp/tests/notification.c
> > > > > > +++ b/dlls/winhttp/tests/notification.c
> > > > > > @@ -44,9 +44,9 @@ struct notification
> > > > > >  {
> > > > > >      enum api function;      /* api responsible for notification */
> > > > > >      unsigned int status;    /* status received */
> > > > > > - int todo;
> > > > > > - int ignore;
> > > > > > - int skipped_for_proxy;
> > > > > > - BOOL todo;
> > > > > > - BOOL ignore;
> > > > > > - BOOL skipped_for_proxy;
> > > > >
> > > > > Do you realize that BOOL is typedef'ed to int and therefore your patches
> > > > > do basically nothing? Is there any real reason behind these patches?
> > > >
> > > > Hi, Dmitry. This has been discussed before, here is an example:
> > > > http://osdir.com/ml/wine-devel/2013-10/msg00131.html
> > >
> > > I don't think that qualifies as a "discussion" :) Besides the using
> > > Windows-only BOOL type these patches break the git history and don't
> > > add any advertized clarity IMHO.
> >
> > Again this is an optimization for the human reader. BOOL, TRUE and FALSE
> > make it easier to read the code without needing too much context.
>
> Is breaking git history really justified with this kind of patches?
> Isn't this from the same (or close enough) area as changing hungarian
> notation or replacing LP(C)STR by (const) char * on the whole Wine code
> base?

Of course it is in the eye of the beholder. For a few of us the benefits
outweigh the git history churn on the BOOL side.
The LPJUNK removal I do it only in the lines I'm already touching with
the exception of the LPIFACE typedefs cause those are even more
annoying.

bye
    michael