Tk Source Code

When building Tk with the latest Windows SDK, there are conflicting
#defines.

The X11/X.h header defines 'None' and 'ControlMask' to literals, and
the latest Windows SDK then attempts to use these as names. The
preprocessor substitution replaces them and compilation fails.

See https://bugs.python.org/issue35402#msg331643 for details and our
discussion on the Python tracker.

I didn't figure out whether the X.h header is necessary on Windows,
and particularly whether it needs to be visible in the Tk.h header. We
can patch our copy of Tk to undefine and redefine these names, but
such a change would need to be upstreamed eventually anyway. For
CPython on Windows, we just used an old Windows SDK to build, but that
isn't sustainable forever.

Asking those involved in this issue to please see [593eb0227] which proposes a way to retain compatibility with code which expects X.h to define ControlMask and None as macros.

Asking those involved in this issue to please see [1c618857968b]. The approach used for this issue relies on an enum whose name is a reserved identifier.

Should be fixed now in all active branches. Closing.

@fvogel, you can review/test [6e620cfa70cf67ff|Branch bug-9e31fd9449-8-6] now, if you want. If no objections, I will merge it to 8.6 (and up) in a few days.

Thanks!

Indeed that one builds.

@fvogel, please try branch  branch bug-9e31fd9449 first, that's the one that is complete. The branch for 8.6 and 8.7 is not adapted yet. (I'm not that fast .....)

I have tried to build branch bug-9e31fd9449-8-6 but I still got a lot of C4047 warnings and it doesn't build since on Windows any warning is a showstopper.

x-ed

Jan, I see you did move the work to branch bug-9e31fd9449. Thanks for that, it really helps!

Well, Fossil is flexible enough to fix any 'messiness' ;-) Done now.

There's a bug-fix branch to be reviewed: https://core.tcl.tk/tk/vdiff?from=aa4f089bc5722290&to=b5d29f4e4068301d. Please review! It contains the minimum changes to get everything compiler warning-free on clang/gcc/MSVC. It's based on core-8-5-branch (I would really like to get this branch a little bit up-to-date first). Later I intend to do the same on other branches.

Let's try chw's measure of 'messyness' here:

$ fossil diff --from core-8-5-branch --to bug-9e31fd9449 |grep ^- |grep None |wc -l

136

Is that more manageable to you?

Thanks!

Branches are really becoming messy.

Now we have Marc Culler fixing sizes [1fcb165e] that were silently (the commit message does not say a word about this) changed [0e4b26fd] as part of this None reform. Marc's fix gives a different size from the one Jan gave, which is again different from the original size of this format[] string. We will have headaches (conflicts) when merging all this.

Please, please, please... could you move all these "None commits" out of the release branches?

Jan is right regarding XCreateGC(). In Xlib it's a pointer to a struct
which itself holds an XID plus other stuff. There's even more ahead.
Take for example XDefineCursor() which wants a Cursor which is an XID
but which is sometimes somewhat wrapped in a Tk_Cursor. OTOH, if Donal's
proposal using careful ifdef'ery and include order works well for all
platforms, it would be the least invasive solution which I'd prefer.

Can I suggest that:

- core-8-5-branch, core-8-6-branch, trunk, revised_text, and all other branches where the changes have already been propagated be *fully* reverted of these changes

- work on the present ticket to take place (only) in a bugfix branch

The present ticket and its partial/controversial fixes definitely leaks too much out of the bugfix branch it should have originally committed into.

Thanks!

The MSVC warning is trying to tell us that "None" cannot be compared to a pointer value, which is right! So, actually, chw's explanation is wrong: XCreateGC() returns a pointer, which is not an XID, so using "None" is inappropriate here, NULL should be used in comparisons . I already fixed various places, in my attempt to bring back "None" as much as possible - as requested - but apparently None is still used inappropriate in more places.

I'll write a more extensive explanation later, but I would like to fix this the right way once and for all. I'm not in favor of replacing None everywhere.

Stay tuned ..... This ticket is not completely done yet.

Surely an easier way would be to add (just after the include of the problem headers on Windows) a sequence like:

#ifdef None
#undef None
#define None 0 // <<--- or whatever the most appropriate thing is
#endif /* None */

Otherwise, a TIP will be mandatory and will face a very rigorous gauntlet.

Moreover core-8-6-branch in its current state cannot be built with MSVC due to a huge number of:

    warning C4047: '==' : 'GC' differs in levels of indirection from 'int'

Not among my habits to say "+1" without more added value, but this deserves I do it.

The current core-8-6-branch compared to things before the complete
elimination of None including the partial revert still gives many
changes as can be easily counted with

 fossil diff --to core-8-6-branch --from 81d20912 |grep ^- |grep None |wc -l

It outputs 328 which isn't exactly a small number of changes (which later
need to be merged into some other development branches). It's very likely
that I'm a too dumb village idiot, and thus too blind to see the added
value compared to version [81d20912].

However, should we really want to fix future name clashes with None,
then we should introduce an X11None or Tk_None symbol, and use it
appropriately. See this discussion for example:

 https://bugzilla.mozilla.org/show_bug.cgi?id=1288686

But we should not change semantics, i.e. XCreateGC() returns an opaque
thing named GC which may be None due to resource constraints. Likewise,
if a GC is contained in a structure component and that GC isn't set,
the component should be assigned the None value and not 0 and not NULL.

And as I asked in my last tcl-core posting, shouldn't that be TIP'ed?

Yes, please let us revert most of the changes I've mentioned in
my last tcl-core posting, hoping that the enum technique works
with the new Windows SDK and can be used as a template, should
Microsoft out of its user and developer friendliness invent
other name clashes in the future. Otherwise, I'll have really
much work to do for AndroWish/undroidwish with its many
integrated extensions.

Actually, I already committed Christian's idea already! So, it should be possible again to use None again, even on Windows.

We are working in other Tk branches on other projects (e.g. this branch), and we are using None in those projects. We are preparing ourselves some problems that will surface at merging time (this may be in the moderately distant future, and at this time we will have forgotten the present ticket, obviously).

I also find it very annoying to need to change hundreds of LOCs.

I didn't understand why Christian's, Brian's, and Gregor's arguments on the Tcl Core list were not answered. They look very sensible to me.

And I didn't understand why chw's (Christian's) proposal below dated 2018-12-21 04:35:09 seems to not have been considered.

Could we please reconsider this situation? Why wouldn't it be possible (and sufficient) to revert those huge commits changing None into 0 in the source code, and apply chw's proposal on top of that?

Although I don't think this ticket can be solved satisfactory for everyone (please blame Microsoft, not me....), making 'None' and 'ControlMask' (as a minimum) into a enum improves the situation. Yes, I consider that a good idea. However going over all the sources again, introducing *Tk_None* (or so) isn't worth the trouble IMHO.

Thanks, Christian! I appreciate your everlasting efforts in making Tk better!

Please consider this technique in ...xlib/X11/X.h:

    #ifdef THE_SHINY_NEW_WINDOWS_SDK
    enum _X11None { None = 0 };
    #else
    #define None 0L
    #endif
    ...
    #ifdef THE_SHINY_NEW_WINDOWS_SDK
    enum _X11ControlMask { ControlMask = (1<<2) };
    #else
    #define ControlMask (1<<2)
    #endif

In theory, this should be the only change required to make things work,
i.e. make it unnecessary to change hundreds of LOCs for the None symbol.
However, it could introduce some compiler warnings from the new shiny
Windows SDK.

Hmm, still not convinced. A less invasive method preserving
somewhat the meaning could have been to rename the problematic
symbols with explicit comments, e.g. None becomes X11_None, and
ControlMask becomes X11_ControlMask.

Note that this is a *** POTENTIAL INCOMPATIBILITY *** for Win32 only: On X11 and Mac, "None" can still be used as before. I don't think much can be done about it: When the newer Microsoft SDK's become more widespread, more and more people will encounter this problem.

It's the same unfortunate change as the function panic(), which turned out to conflict with newer MacOSX headers: On win32 and UNIX, extensions using panic() compile fine, but extensions which want to support MacOSX will need to change those calls to Tcl_Panic().

Suggestion: Please build some extensions (e.g. the ones provided by Androwish) with a newer Microsoft SDK and see what happens. Then come back here and share your experience with us ;-) If you find out a better idea, I for sure want to hear all about it!

Anyway, this should be fixed now in all active branches, starting with core-8-5-branch.

IMHO check-in [8a96bfd0] and friends are too invasive since usage of None
in Xlib calls is well known practice and as such found through all ancient
documentation. There must be a better way to deal with the new bright
shiny Windows SDKs. In other words: the advent of a new born Windows SDK
should not require us to rewrite entire history.