Include win32defines.h before win32.h to fix build errors

[eht16]

This should fix the problems mentioned in #567.

However, I don't get completely why win32.h includes _mingw.h or any other non-Geany headers.

[b4n]

LGTM, and should fix @zhekov issue as he described.

Maybe also define #define _WIN32_WINNT 0x0501 as he suggested? Seems similar/the same as WINVER it it probably wouldn't hurt (https://msdn.microsoft.com/en-us/library/windows/desktop/aa383745%28v=vs.85%29.aspx). But maybe later, in case it has unexpected implications.

[kugel-]

Under which conditions are build errors happening? I'm not seeing those.

[eht16]

@kugel- I don't see errors neither but warnings. @zhekov mentioned build errors in #567.

And closely reading the warning message, I got the include path of non-Geany headers:

In file included from ..\src\win32.c:34:0:
..\src\win32defines.h:27:0: warning: "WINVER" redefined [enabled by default]
 #define WINVER 0x0501
 ^
In file included from C:/perl/c/i686-w64-mingw32/include/crtdefs.h:10:0,
                 from C:/perl/c/i686-w64-mingw32/include/stdio.h:9,
                 from c:\git\geany\tagmanager\src/tm_source_file.h:13,
                 from c:\git\geany\tagmanager\src/tm_tag.h:32,
                 from ..\src\editor.h:26,
                 from ..\src\document.h:32,
                 from ..\src\win32.h:25,
                 from ..\src\win32.c:30:
C:/perl/c/i686-w64-mingw32/include/_mingw.h:225:0: note: this is the location of the previous definition
 #define WINVER 0x0502

@b4n, @zhekov: I'm not sure about defining _WIN32_WINNT as well. I don't know the exact difference between both and so I'm somewhat girlish minutes before 1.25.

[kugel-]

My _mingw.h has

224: #ifndef _WIN32_WINNT
225: #define _WIN32_WINNT 0x502
226: #endif

But no sign of WINVER anywhere. Anyway it suggest we should define this stuff before the system headers (which seem to have an #ifndef guard before redefining)

[eht16]

http://blogs.msdn.com/b/oldnewthing/archive/2007/04/11/2079137.aspx describes both a bit.
According to this, I'd add _WIN32_WINNT as well and set it to the same value as WINVER now.

[eht16]

Anyway it suggest we should define this stuff before the system headers (which seem to have an #ifndef guard before redefining)

Which is what we do now with this change.

[b4n]

Adding _WIN32_WINNT adds more:

In file included from ../../src/spawn.c:52:0:
../../src/win32defines.h:28:0: warning: "_WIN32_WINNT" redefined
 #define _WIN32_WINNT 0x0501
 ^
In file included from /usr/share/mingw-w64/include/crtdefs.h:10:0,
                 from /usr/share/mingw-w64/include/errno.h:9,
                 from ../../src/spawn.c:46:
/usr/share/mingw-w64/include/_mingw.h:225:0: note: this is the location of the previous definition
 #define _WIN32_WINNT 0x502
 ^

But arguably the same header inclusion move should happen in all including files.

[kugel-]

Which is what we do now with this change.

I mean before windows.h and friends, what we do already without this
change (I thought someone suggested to move it behind).

[b4n]

Adding _WIN32_WINNT adds more:

Hold that, it's the same with WINVER on some mingw. From Travis:

In file included from /home/travis/build/geany/geany/src/socket.c:82:0:
/home/travis/build/geany/geany/src/win32defines.h:27:0: warning: "WINVER" redefined [enabled by default]
/usr/lib/gcc/i686-w64-mingw32/4.6/../../../../i686-w64-mingw32/include/_mingw.h:240:0: note: this is the location of the previous definition
/home/travis/build/geany/geany/src/win32defines.h:28:0: warning: "_WIN32_WINNT" redefined [enabled by default]
/usr/lib/gcc/i686-w64-mingw32/4.6/../../../../i686-w64-mingw32/include/_mingw.h:244:0: note: this is the location of the previous definition

[zhekov]

We have a # include "win32defines.h" in spawn? Please remove it, or move it before any other includes. Spawn requires neither WINVER nor WIN32_LEAN_AND_MEAN, and "win32defines.h" certainly can't be included based on glib.h-s G_OS_WIN32, because the glib.h headers include tons of stuff, and WINVER will be 100% surely to be defined at that point.

[eht16]

@zhekov already fixed with the last commits: the include of win32defines.h is moved at the top.
So far I guess the include itself won't hurt so let's keep it.

[b4n]

@zhekov before win32defines.h those were defined differently depending on the build system -- which actually made Autotools fail on Windows because it didn't have any. Waf and Windows makefiles however had (most of) them in config.h.

[zhekov]

On 12.7.2015 г. 17:58, Colomban Wendling wrote:

LGTM, and should fix @zhekov https://github.com/zhekov issue as he
described.

Maybe also define |#define _WIN32_WINNT 0x0501| as he suggested? Seems
similar/the same as |WINVER| it it probably wouldn't hurt

Well, MinGW checks _WIN32_WINNT for SHGetFolderPathAndSubDir, but
automatically defines _WIN32_WINNT to WINVER if not defined, at least in
my winver.h, so it shoudn't matter.

(To whoever might be interested, the MS SDK 7.0A checks NTDDI_VERSION
for SHGetFolderPathAndSubDir, and defines NTDDI_VERSION based on
_WIN32_WINNT if not defined, but _WIN32_WINNT is, at least sometimes,
0x0500 by default, regardless of WINVER.)

E-gards: Jimmy

[zhekov]

@b4n

Hold on.

Most sources do not include "win32defines", but they include at least some standard headers under Windows, which means that WINVER etc. will be defined, and will receive their automatic values. spawn.c including windows.h is no different from build.c including stdlib.h - WINVER etc. will be defined in both cases.

Thus being the case, there are 2 proper solutions:

1. Make autotools define WINVER etc. when generation config.h, if we want these defines to be the same everywhere (not needed AFAIK).
2. Include "win32defines" only in the modules which actually require WINVER etc. That is, inline them into win32.c.

The current approach, "include win32defines because something seems related to Windows", does not make much sense.

[b4n]

My problem with 1 is that we have 3 build systems, so this means 3 versions of the same defines to keep in sync. Also, they seemed somewhat like code stuff to me.

2 why not, that's what I would have done but @codebrainz wanted to have them all the same when we included windows.h, and it seemed reasonable to me -- but I have really no clue about Windows details, I'm mostly happy when it works.

So I really don't mind. But is it a problem how it's done now? I mean, would it create any issue, or is it just a bit crazy/unnecessary but without too much incidence?

BTW, IIUC the main goal of having win32defines everywhere windows.h is included was to define the LEAN stuff so windows.h brings less unnecessary stuff (but I might be wrong).

[b4n]

@zhekov BTW I was about to upload the 1.25 tarballs so if it'd be a real problem I'd like to know like now :)

[zhekov]

No, it will not be a problem AFAICT. Go on!

I'll create a PR after the release, because:

1. "Include what you use" (and @codebrainz cares about unneeded exports as well).
2. If we define them in modules which don't need them, why at least not make them the same for all modules?

[codebrainz]

but @codebrainz wanted to have them all the same

My actually suggestion was below (pasted from closed PR #539), for the record :)

Begin pasted text...

I mean like this, in a geanywin32.h or win32private.h file or something:

#pragma once
# ifdef _WIN32
#  define WIN32_LEAN_AND_MEAN
#  define VC_EXTRALEAN
#  define WINVER 0x0501
#  define _WIN32_IE 0x0500
#  include <windows.h>
#  include <commdlg.h>
#  include <shellapi.h>
#  include <shellobj.h>
#  include <winsock2.h>
#  include <gdk/gdkwin32.h>
// ...
# endif

It would remove several lines from each file that uses Win32 API and make it more robust against forgetting to include the win32defines stuff or accidentally including it after windows.h where it does nothing. Kind of similar idea as gtk/gtk.h in a way.

[zhekov]

I see. Should have noticed #539 and discussed it at the time, but I missed it...

The code suggested by you would have been more logical than what we ended up with. It's problems (as of me) are:

• it uses WIN32 to check for Windows, while the entire Geany is G_OS*
• what headers to consider common? A "banana" problem.

I plan to create a PR about win32defines.h, and to first explain in detail about WINVER and _WIN32_WINNT, when they should be defined etc. There are a lot of misconceptions, and Geany is written mostly by *nix programmers...

[codebrainz]

it uses WIN32 to check for Windows, while the entire Geany is G_OS*

If I understood correctly it _WIN32 is defined by the compiler itself. If not, perhaps like __MINGW32__ or whatever appropriate compiler-defined (ie. doesn't require #includeing any GLib headers first) would make most sense.

what headers to consider common? A "banana" problem

My thought was any win32-specific headers, especially those of the official Win32 API (as opposed to GLib/GTK+ stuff). It causes some extra unnecessary inclusions, but I doubt it's a big deal, and IMO "include what you use" is more important for our own headers than external libraries'.

[eht16]

There are a lot of misconceptions, and Geany is written mostly by *nix programmers...

So true, at least for me.
I'd be happy to have this sorted out by someone who knows what he does :).

The current solution was a) because we don't know what we do on Windows
and b) we wanted to release today.
But if we get it right for the next release, that'd be great.

[zhekov]

@codebrainz

Well, defining WIN32_LEAN_AND_MEAN + VC_EXTRALEAN, while at the same time including some of the headers excluded by them, is a bit strange. Shouldn't we avoid trying to guess precisely what the Win32 developers will/may need, and let them specify it?

[zhekov]

Created PR #573 with a long explanation about this Win~1 stuff, as planned.