Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: >> -# ifdef HAVE_LIBCHARSET_H >> +# ifdef GIT_WINDOWS_NATIVE >> + ... new windows-only code ... >> +# elif defined HAVE_LIBCHARSET_H >> # include <libcharset.h> >> # else >> # include <langinfo.h> > ... > It looks to me that with this patch the HAVE_LIBCHARSET_H docs in > "Makefile" become wrong. Shouldn't those be updated too? I do not think this change has much to do with HAVE_LIBCHARSET_H; it inserts "regardless of what we have been doing, do this new thing only and always on windows (persumably '... because libcharset would not be useful on that platform')". Existing users of HAVE_LIBCHARSET_H and existing non-windows users that did not use HAVE_LIBCHARSET_H are not affected, and whatever Makefile documents the macro as still applies to them. > I wonder if it wouldn't be better to always compile this function, and > just have init_gettext_charset() switch between the two. We've moved > more towards that sort of thing (e.g. with pthreads). I.e. prefer > redundant compilation to ifdefing platform-only code (which then only > gets compiled there). See "HAVE_THREADS" in the code. OK, so init_gettext_charset() is the only caller of locale_charset() in our codebase, and we supply our own locale_charset() if we do not have <libcharset.h>, either with nl_langinfo(), or with the code introduced by the patch in question for windows. Your suggestion is to add a block of #ifdef cascade in init_gettext_charset() to call locale_charset(), nl_langinfo(), or the windows-only code (perhaps inlined right there)? I am not sure. We'd need the conditional inclusion of header files depending on HAVE_LIBCHARSET_H etc. anyway, so...
