GNOME Bugzilla – Bug 660095
Incorrect pre-processor conditions used in gdir.c
Last modified: 2018-05-24 13:25:31 UTC
Created attachment 197440 [details] [review] Fix pre-processor conditions in gdir.c gdir.c uses one particular set of pre-processor conditions to include wdirect.c and related header file, and a completely different condition when using it. This is not useful and break the build. When including wdirect.c it uses: #if defined (_MSC_VER) && !defined (HAVE_DIRENT_H) which is ok - if you are using the Microsoft compiler and you don't have dirent.h ... but then everywhere in the code where it then uses the wdirect stuff it uses #ifdef G_OS_WIN32 which is incorrect. What if you are using MSVC but you DO have dirent.h? The attached patch fixes this so the same condition that it used to include the code is used when using it, which is how it should be.
Review of attachment 197440 [details] [review]: Hi Kean, Thanks for the patch, I think it seems fine on my part, will ask Ryan (desrt) whether it's ok to apply this patch though at night time here later on. Something to note, by the way: -MSVC builds have HAVE_DIRENT_H disabled by default (see config.h.win32(.in), the config.h used by the MSVC build process), unless one edits the file to enable that Thank you, and God bless!
Yeah I know config.h.win32 leaves dirent.h undefined but I have an environment where I always have dirent.h, inttypes.h and stdint.h present, regardless of what version of MSVC I am using so I do edit that file before building glib. I am just waiting for a final 2.30 and to finish up some testing and I'll be mailing the list about said environment.
Review of attachment 197440 [details] [review]: Hi Kean, It seems to me like your patch changes things when you are not using MSVC, for instance when you use a mingw compiler. Then _MSVC is not defined but G_OS_WIN32 is defined.
Review of attachment 197440 [details] [review]: That is by design. Mingw has the dirent() functions in it's library. The changes *only* apply to MSVC.
(In reply to comment #4) > The changes *only* apply to MSVC. No, your changes affect all windows compilers. If that is what you intend to do with this patch, please make that clear and give your reasons for that change. I have no opinion whether it would be better to use either "_WDIR *wdirp;" or "DIR *dirp;" on Windows, but your patch seems to change this from the former to the latter for all windows compilers except for MSVC.
If you use MSVC but you have dirent.h, you should still be using the wide-character versions of the dirent functions. Or does your dirent not provide them? In that case more ifdef complexity would be needed. Why bother, why not just build in the (very small) included dirent code in GLib also in the MSVC? And anyway, I doubt it makes sense to try to cover all possible combinations of "use MSVC, but have this or that 3rd-party open source thing anyway". Isn't the MSVC support in the GTK+ stack supposed to cover using MSVC out of the box, not with an arbitrary collection of add-on stuff, as it after all uses a pre-generated config.h.win32(.in), doesn't it? There is no configure script to run when building with MSVC.
-- 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/457.