On Mon, Apr 21, 2025 at 01:51:45PM -0400, Eli Schwartz wrote:
> This is repetitive logic. We either want to use some -lc function, or if
> it is not available we define it as -DNO_XXX and usually (but not
> always) provide some custom compatibility impl instead.
>
> Checking the intent of each block when reading through the file is slow
> and not very DRY. Switch to taking an array of checkable functions
> instead.
>
> Not all functions are straightforward to move, since different macro
> prefixes are used.
Yeah, this is somewhat unfortunate indeed. I think in the long term we
might want to unify our approach so that we consistently use e.g.
`HAVE_SOME_FUNCTION` or `NO_SOME_FUNCTION`.
> Signed-off-by: Eli Schwartz <eschwartz@xxxxxxxxxx>
> ---
> meson.build | 73 ++++++++++++++++++++++-------------------------------
> 1 file changed, 30 insertions(+), 43 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index c47cb79af0..6c147c22a4 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -1290,23 +1290,40 @@ if not compiler.has_member('struct passwd', 'pw_gecos', prefix: '#include <pwd.h
> libgit_c_args += '-DNO_GECOS_IN_PWENT'
> endif
>
> -if compiler.has_function('sync_file_range')
> - libgit_c_args += '-DHAVE_SYNC_FILE_RANGE'
> -endif
> +checkfuncs = [
> + 'strcasestr',
> + 'memmem',
> + 'strlcpy',
> + # no compat
> + 'strtoull',
> + 'setenv',
> + 'mkdtemp',
> + # no compat
> + 'initgroups',
> +]
>
> -if not compiler.has_function('strcasestr')
> - libgit_c_args += '-DNO_STRCASESTR'
> - libgit_sources += 'compat/strcasestr.c'
> +if host_machine.system() == 'windows'
> + libgit_c_args += '-DUSE_WIN32_MMAP'
> +else
> + checkfuncs += [
> + 'mmap',
> + # unsetenv is provided by compat/mingw.c.
> + 'unsetenv',
> + ]
> endif
>
> -if not compiler.has_function('memmem')
> - libgit_c_args += '-DNO_MEMMEM'
> - libgit_sources += 'compat/memmem.c'
> -endif
> +foreach func: checkfuncs
Our current code style puts a space both before and after the colon.
> + if not compiler.has_function(func)
> + libgit_c_args += '-DNO_' + func.to_upper()
> + impl = 'compat/' + func + '.c'
> + if fs.exists(impl)
> + libgit_sources += impl
> + endif
I think this is a bit too magic-y. An alternative might be to have
`checkfuncs` be a dictionary where the value of each function is the
compat sources that we fall back to.
Other than that I really like the simplifications introduced by this
patch, thanks!
Patrick
