search

SWI-Prolog / packages-xpce

The graphics toolkit for SWI-Prolog
16 stars 14 forks source link

Revert "FIX: conflicting declarations" #37

Closed mgondan closed 2 months ago

mgondan commented 2 months ago

Reverts SWI-Prolog/packages-xpce#36

JanWielemaker commented 2 months ago

Could you be more specific on what is going on? What error is reported and why you think that should be fixed as you intend? I see this code is still using K&R syntax. Could it be that it needs updating to ansi style declarations? I updated a lot of K&R code a couple of years ago. Probably for clang, though I do not recall. Note that this is Windows specific code and clang has never been used for the Windows version. It is also a bad idea to use clang as it makes SWI-Prolog quite a bit slower ...

mgondan commented 2 months ago

I wasn't aware that you get notified of "draft" PR. Sorry for the confusion.

This revert-PR isn't yet ready. I am currently investigating the problem. I just noticed that PR #36 was incomplete (apologies for this).

mgondan commented 2 months ago

Just to make sure (since I am unfamiliar with "reverting PRs"). The code should now look like this:

create.c, line 51: (see https://www.xfree86.org/current/xpm.pdf)

LFUNC(AllocColor, int, (Display *display, Colormap colormap,
            char *colorname, XColor *xcolor, void *closure));

line 220 accordingly:

static int
AllocColor(display, colormap, colorname, xcolor, closure)
    Display *display;
    Colormap colormap;
    char *colorname;
    XColor *xcolor;
    void *closure;      /* not used */
{
    int status;
    if (colorname)
    if (!XParseColor(display, &colormap, colorname, xcolor))
        return -1;
    status = XAllocColor(display, &colormap, xcolor);
    return status != 0 ? 1 : 0;
}

Note the &colormap in l. 230 and l. 232 (see simx.h).

JanWielemaker commented 2 months ago

I'm a bit confused. As the patch broke compilation, I reverted it (pushed). It is clear something fishy is going on. Introducing &colormap goes from Colormap * to Colormap **, so I think that is not right either. I do not have clang for windows or its linux corss-compiler equivalent.

Not sure where to go. Best would be to convert the code to modern C. Sooner or later that needs to happen, but it is quite a bit of work. It would probably result in consistent error messages. Otherwise the simplest option for now is probably to claim non-portability to clang on Windows. It is a bad choice anyway.

I'm very reluctant to touch this old code as long as it works on gcc and msvc.

mgondan commented 2 months ago

Introducing &colormap goes from Colormap * to Colormap **, so I think that is not right either.

Actually, no: from Colormap to Colormap *

JanWielemaker commented 2 months ago

Actually, no: from Colormap to Colormap *

I guess we are looking at different things :cry: It is hard to comment without proper context and error messages ...

mgondan commented 2 months ago

I was referring to the code from my comment above—which I hope is the result of this PR when merged. If needed, I'll provide more context tomorrow (including error messages from clang).

JanWielemaker commented 2 months ago

Yes, please turn it into a clean PR (it no longer applies) and include the clang error before the patch here.

JanWielemaker commented 2 months ago

Ok. Used some script to convert most of the K&R style functions to use ANSI C. That also exposed the issue for gcc. Fixed. Unfortunately the script is not that good and missed quite a few functions, so there is still K&R code in there :cry: At least some of the typing is done ...

mgondan commented 2 months ago

Nice! So I close that case. Thank you