On 15/02/2022 23:40, Ævar Arnfjörð Bjarmason wrote:
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.
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
