Re: [PATCH 1/1] xdiff: provide indirection to git functions

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Feb 16, 2022, 11:02 UTC
Message-ID: <7e6385f8-f25d-69f5-edae-6f5d6f785046@gmail.com>
In-Reply-To: <220216.86wnhvvgeh.gmgdl@evledraar.gmail.com>

On 15/02/2022 23:40, Ævar Arnfjörð Bjarmason wrote:

Show 25 quoted lines

> 
> On Wed, Feb 09 2022, Edward Thomson wrote:
> 
>> Provide an indirection layer into the git-specific functionality and
>> utilities in `git-xdiff.h`, prefixing those types and functions with
>> `xdl_` (and `XDL_` for macros).  This allows other projects that use
>> git's xdiff implementation to keep up-to-date; they can now take all the
>> files _except_ `git-xdiff.h`, which they have customized for their own
>> environment.
> 
> It seems sensible to share code here, but...
> 
>> +#ifndef GIT_XDIFF_H
>> +#define GIT_XDIFF_H
>> +
>> +#define xdl_malloc(x) xmalloc(x)
>> +#define xdl_free(ptr) free(ptr)
>> +#define xdl_realloc(ptr,x) xrealloc(ptr,x)
> 
> ...I don't understand the need for prefixing every function that may be
> used from git.git with xdl_*. In particular for these memory managing
> functions shouldn't this Just Work per 8d128513429 (grep/pcre2: actually
> make pcre2 use custom allocator, 2021-02-18) and cbe81e653fa
> (grep/pcre2: move back to thread-only PCREv2 structures, 2021-02-18)?
> I.e. link-time use of free().

I read that paragraph a couple of times and I'm still not sure I understand what you're saying. It is not unusual for libraries to define their own allocation functions and the code base is already using xdl_malloc etc so these defines seem quite reasonable. As you point out below we'd need wrappers for xmalloc() etc anyway so I'm not sure what the problem is.

Show 32 quoted lines

> Of course trivial wrappers would be needed for x*() variants...
> 
>> +#define xdl_regex_t regex_t
> 
> This is a type that's in POSIX. Why do we need an xdl_* prefix for it?
> 
>> +#define xdl_regmatch_t regmatch_t
> 
> ditto.
> 
>> +#define xdl_regexec_buf(p, b, s, n, m, f) regexec_buf(p, b, s, n, m, f)
> 
> But this is our own custom function, which brings me to...
> 
>> +#define XDL_BUG(msg) BUG(msg)
> 
> ...unless libgit2 has a regexec_buf() or BUG() why do we need this
> indirection? Let's just have xdiff() use a bug, and then either libgit2
> will have a BUG() macro/function, or it'll fail at compile-time.
> 
> This seems to at least partly have been inspired by git.git's
> 546096a5cbb (xdiff: use BUG(...), not xdl_bug(...), 2021-06-07), i.e. we
> used to have an xdl_bug(), but now we just use BUG().
> 
> I then see on your libgit2 side 1458fb56e (xdiff: include new xdiff from
> git, 2022-01-29).
> 
> But why not simply?:
> 
>      #define BUG(msg) GIT_ASSERT(msg)
> 
> It would make things easier on the git.git side (etags and all).

If we want xdiff to be usable for other projects I think we're going to have to accept that it is sensible to namespace its functions.

Best Wishes

Phillip