Re: [RP] ratpoison-1.4.6 bug report

[Jeff Abrahamson]

On 14 September 2014 20:51, Jérémie Courrèges-Anglas <address@hidden> wrote:

> On 8 September 2014 21:30, David Binderman <address@hidden> wrote:
>
>> Hello there,
>>
>> This bug is also in trunk. I just checked.
>>
>> xinerama.c:50:56: warning: logical not is only applied to the left hand
>> side of comparison [-Wlogical-not-parentheses]
>>
>>         if (!XineramaQueryVersion(dpy, &major, &minor) != Success) {
>>                 return;
>>         }
>>
>> Clearly some dubious sanity checking code. Suggest code rework.

Dubious, but how incorrect is it?

I'm not clear that it's incorrect, but I'll readily agree that the more I look at it, the less I like it. I was initially tempted to fix it just by adding the parentheses that don't change meaning but do make it impossible for a human to mis-parse.

But I don't think that would be the Right Thing To Do. From the XineramaQueryVersion man page:

       If  the  Xinerama  library is compatible with the version returned by the server,
       it returns nonzero. If the server does not support the XINERAMA extension, or if there

       was an error during communications with the server, or if the server and library

       protocol  versions  are incompatible, it returns zero.

Success is defined in /usr/include/X11/X.h with value zero. But here zero is the error value, so I'm not keen on using the symbolic constant Success, since it is both at odds with the manpage and with the sense of what is happening. If XineramaQueryVersion() returns non-zero, it means that we correctly found the xinerama major and minor version numbers. If it returns 0 (Success), then it means we've failed and should drop out of xinerama_init() without setting rp_have_xinerama.

I think, therefore, that the correct line should be

        if (!XineramaQueryVersion(dpy, &major, &minor)) {

                return;

        }

which, admittedly (and thankfully), must have the same behavior.

I'll submit a patch soon-ish.