GNOME Bugzilla – Bug 711547
win32: silence some build warnings
Last modified: 2018-05-24 15:50:18 UTC
ssia
Created attachment 259071 [details] [review] win32: silence deprecation warning gwin32.c: In function 'g_win32_get_package_installation_subdirectory': gwin32.c:486:3: warning: 'g_win32_get_package_installation_directory' is deprecated (declared at gwin32.c:402) [-Wdeprecated-declarations] prefix = g_win32_get_package_installation_directory (package, dll_name);
Created attachment 259072 [details] [review] win32: silence a mingw warning gstdio.c: In function 'g_stat': gstdio.c:496:3: warning: passing argument 2 of '_wstat' from incompatible pointer type [enabled by default] retval = _wstat (wfilename, buf); ^ In file included from gstdio.c:27:0: /usr/i686-w64-mingw32/sys-root/mingw/include/sys/stat.h:126:66: note: expected 'struct _stat *' but argument is of type 'struct GStatBuf *' _CRTIMP int __cdecl _wstat32(const wchar_t *_Name,struct _stat32 *_Stat);
Created attachment 259073 [details] [review] win32: silence build warning glocalfileinfo.c: In function '_g_local_file_info_get': glocalfileinfo.c:1955:11: warning: passing argument 3 of 'get_thumbnail_attributes' from incompatible pointer type [enabled by default] get_thumbnail_attributes (path, info, &statbuf); ^ glocalfileinfo.c:1285:1: note: expected 'const struct GStatBuf *' but argument is of type 'struct _stati64 *' get_thumbnail_attributes (const char *path,
Created attachment 259074 [details] [review] win32: silence build warning gdbusauthmechanismexternal.c: In function 'mechanism_client_initiate': gdbusauthmechanismexternal.c:355:3: warning: 'initial_response' may be used uninitialized in this function [-Wmaybe-uninitialized] return initial_response; ^ gdbusauthmechanismexternal.c:332:10: note: 'initial_response' was declared here
Created attachment 259075 [details] [review] win32: silence build warning glocalfile.c: In function 'g_local_file_measure_size_of_file': glocalfile.c:2654:3: warning: passing argument 2 of 'g_lstat' from incompatible pointer type [enabled by default] if (g_lstat (name->data, &buf) != 0) ^ In file included from glocalfile.c:68:0: ../glib/gstdio.h:135:5: note: expected 'struct GStatBuf *' but argument is of type 'struct _stati64 *'
Comment on attachment 259071 [details] [review] win32: silence deprecation warning >- prefix = g_win32_get_package_installation_directory (package, dll_name); >+ prefix = _g_win32_get_package_installation_directory (package, dll_name); an easier fix is: + G_GNUC_BEGIN_IGNORE_DEPRECATIONS prefix = g_win32_get_package_installation_directory (package, dll_name); + G_GNUC_END_IGNORE_DEPRECATIONS
Comment on attachment 259073 [details] [review] win32: silence build warning seems right
Comment on attachment 259075 [details] [review] win32: silence build warning >- struct stat buf; >+ GStatBuf buf; not GLocalFileStat?
Comment on attachment 259072 [details] [review] win32: silence a mingw warning (I have no clue on this one...)
This may be related to https://bugzilla.gnome.org/show_bug.cgi?id=669818 Anyway, the stat warning is more than just a warning. What some people don't quite get is that mingw-w64 supports LFS, and supported it for some time (a couple of years, roughly). LFS is controlled by an intricate set of macros, and there's a blog post[1] i did to explore and systematize this, but in a nutshell: normal autotooled software packages will end up using LFS, with "struct stat" having 64-bit st_size field, regardless of OS bitness, while time fields (an issue orthogonal to LFS) will be 32-bit on 32-bit OS and 64-bit on 64-bit OS. gstdio.h, as of March 2010, typedefs GStatBuf as "struct stat" instead of "struct _stat". Which means that GStatBuf will have LFS, if configure is used (I.e. glib is built in MSYS, or cross-compiled from GNU/Linux or Cygwin). "struct _stat" would not have had this problem, as it has 32-bit st_size field, always. Therefore, i think that the patch for gstdio.h (which adds mingw to the group of platforms for which struct _stat32 is used instead of struct stat) is correct. [1] https://gnunet.org/content/sorting-out-stat-thing
OK, the patch can be improved slightly by swapping __MINGW32__ for __MINGW64_VERSION_MAJOR (because there's no "_stat32" in mingw.org as far as i can see).
Created attachment 270707 [details] [review] Expand forced 32-bit GStatBuf definition to mingw-w64
Created attachment 270710 [details] [review] Use 64-bit stat function for GLocalFile size calculation, not g_l?stat g_stat() is 32-bit only (see the other patch), whereas GLocalFileStat is always 64-bit. g_lstat() just calls g_stat() on W32, so that's not an option.
Created attachment 270711 [details] [review] Fix warnings (uninitialized variables), unused variable Since this bug is about silencing warnings, here's a patch that silences various warnings: * struct in_addr ip4addr; is unused on W32 (inet_aton() is not called) * poll_timer is not initialized (gcc is not smart enough to understand that all branches where poll_timer are used also have it initialized) * hmodule is not initialized (same as poll_timer) * wc is not initialized (initialize it to something that fails)
Review of attachment 259073 [details] [review]: Hi, Unfortunately this breaks the thumbnail-verification test on Windows... When I reverted this patch, the test runs successfully (albeit with a C4133 warning when building GIO, which I think should be like the warning that Marc-Andre is getting)
Review of attachment 270711 [details] [review]: The pacthes for gresolver, gmain and gutils are obvious and should be pushed right away. The one for win_iconv looks ok to me, but I am not familiar enough with that code to say if I am missing something
(In reply to comment #16) > Review of attachment 270711 [details] [review]: > > The pacthes for gresolver, gmain and gutils are obvious and should be pushed > right away. > > The one for win_iconv looks ok to me, but I am not familiar enough with that > code to say if I am missing something AFAIU, utf32_mbtowc() converts a multibyte character (stored in buf, up to 4 bytes long) to a wide character. It knows two codepage groups - 12000 and 12001, with different endianness. If codepage group is 12000, it forms an UCS4 char in LE. If codepage group is 12001, it forms the UCS4 char in BE. It then checks that the UCS4 char is in legal range, then calls ucs4_to_utf16() on it. The problem is that there are other codepage groups beside 12000 and 12001. Now, i haven't checked the code thoroughly, but it appears that glib *first* checks the codepage, then assigns one of the conversion functions (such as utf32_mbtowc) to a placeholder variable, which later is called. So it seems that it's guaranteed that utf32_mbtowc() will only be called for codepage groups 12000 and 12001 (but gcc doesn't get it, sadly). Thus initializing uint wc to 0xD800 just placates gcc, wc never actually gets used without being set. If your conscience is still troubled, it should be possible to add an assert that crashes if cv->codepage is not 12000 or 12001.
I think the explanation makes sense. I'd still prefer the two patches to be pushed separately so that the one that's more controversial is easier to bisect if that's ever needed. Anyway go ahead.
Created attachment 282329 [details] [review] Silence some uncontroversial warnings
Created attachment 282330 [details] [review] Silence a controversial warning in win_iconv
Attachment 282329 [details] pushed as 42ddcc6 - Silence some uncontroversial warnings Attachment 282330 [details] pushed as 7e0cb48 - Silence a controversial warning in win_iconv
Comment on attachment 270710 [details] [review] Use 64-bit stat function for GLocalFile size calculation, not g_l?stat Obsoleting this attachment (committed, but in another bug).
Created attachment 302130 [details] [review] Fix the thumbnail verification test on Windows Hi, For me, first I would like to make sure things are also functional with Visual Studio builds after items in this patch :). (In reply to Fan, Chun-wei from comment #15) > Review of attachment 259073 [details] [review] [review]: > > Hi, > > Unfortunately this breaks the thumbnail-verification test on Windows... When > I reverted this patch, the test runs successfully (albeit with a C4133 > warning when building GIO, which I think should be like the warning that > Marc-Andre is getting) Probably the above item didn't reach people, but I think I have a fix for it, for the test itself... Make sure that the types in the test are in-line with the updates we have in attachment 259073 [details] [review]. With blessings, thank you!
Review of attachment 270707 [details] [review]: Hi, I would definitely not be the best person to say whether this or attachment 259072 [details] [review] is better, as probably people know that I am more Visual Studio-centered. As with attachment 259072 [details] [review], I guess this narrows down to the MinGW situation, which AFAICT is not in a very uniform situation (and is in a situation that I am not familiar with, due to the existence of original mingw.org and mingw-w64, which actually complicated things for the right (or better) check macro. I think the best way to go with this is to see whether there are still many people trying to build with mingw.org, and go with the macro that most GCC-Windows (be it mingw.org or mingw-w64) will use, and possibly let people know in some sort of README.win32 (or so), that if they choose to go with the GCC route, they are recommended (or down the road, required, even) to use that flavour of GCC-Windows. My take on this. With blessings, thank you!
Review of attachment 259072 [details] [review]: (Please see my review of 270707 about my take on this).
(In reply to Fan, Chun-wei from comment #24) > Review of attachment 270707 [details] [review] [review]: > > Hi, > > I would definitely not be the best person to say whether this or attachment > 259072 [details] [review] is better, as probably people know that I am more > Visual Studio-centered. > > As with attachment 259072 [details] [review] [review], I guess this narrows down to > the MinGW situation, which AFAICT is not in a very uniform situation (and is > in a situation that I am not familiar with, due to the existence of original > mingw.org and mingw-w64, which actually complicated things for the right (or > better) check macro. I think the best way to go with this is to see whether > there are still many people trying to build with mingw.org, and go with the > macro that most GCC-Windows (be it mingw.org or mingw-w64) will use, and > possibly let people know in some sort of README.win32 (or so), that if they > choose to go with the GCC route, they are recommended (or down the road, > required, even) to use that flavour of GCC-Windows. AFAIU, this actually is not about choosing the right macro to identify MinGW. This is about choosing the right macro to identify mingw-w64, because this condition must not come true for mingw.org, because there's no "_stat32" in mingw.org as far as i can see. I don't have a functioning mingw.org installation to test this though. Anyway, if my understanding is correct and mingw.org has no _stat32, then using __MINGW64_VERSION_MAJOR is the right thing to do. If mingw.org has _stat32, then __MINGW32__ is the macro to use. All boils down to this.
Review of attachment 302130 [details] [review]: This, obviously, is correct.
Review of attachment 302130 [details] [review]: Hello LRN, Thanks for checking out the patch, the patch was pushed as 10b5a8be (master) and 5afaa31 (glib-2-44). (In reply to LRN from comment #26) > AFAIU, this actually is not about choosing the right macro to identify > MinGW. This is about choosing the right macro to identify mingw-w64, because > this condition must not come true for mingw.org, because there's no > "_stat32" in mingw.org as far as i can see. I don't have a functioning > mingw.org installation to test this though. Anyway, if my understanding is > correct and mingw.org has no _stat32, then using __MINGW64_VERSION_MAJOR is > the right thing to do. If mingw.org has _stat32, then __MINGW32__ is the > macro to use. All boils down to this. Yup, this is what I am getting at in comments 24/25. Thanks for making the issue more clear to people. With blessings, thank you!
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/777.