Hi Kyle
On 09/05/2024 02:00, Kyle Lippincott wrote:
// RENAMED from previous code block (no other changes)
// In a .c file that is "library internal".
// This translation unit can assume that we've done #include
"git-compat-util.h" and anything else it wants.
int strbuf_grow_impl(struct strbuf *sb, size_t extra)
{
int new_buf = !sb->alloc;
if (unsigned_add_overflows(extra, 1) ||
unsigned_add_overflows(sb->len, extra + 1))
return -1;
if (new_buf)
sb->buf = NULL;
ALLOC_GROW(sb->buf, sb->len + extra + 1, sb->alloc);
if (new_buf)
sb->buf[0] = '\0';
return 0;
}
// In a .c file for the interface as used by other projects:
int gitlib_strbuf_grow(struct strbuf *sb, size_t extra)
{
return strbuf_grow_impl(sb, extra);
}
// NEW from previous code block
// In a .c file for the interface as used by the git project itself:
void strbuf_grow(struct strbuf *sb, size_t extra)
{
if (strbuf_grow_impl(sb, extra))
die("you want to use way too much memory")
}
I'm recommending this pattern primarily because of our platform
support concerns. If we can't elevate the entire project to assume
that C99 is available in a standards compliant way, we can't have
header files that look like this be part of building the `git` binary
itself (or any of the helper binaries):
#include <stdint.h> /* Our platform support policy doesn't allow this */
int gitlib_strbuf_grow(struct strbuf *sb, size_t extra);
We have had a test balloon [1] requiring C99 for two and a half years
without any bug reports so I think we are probably safe to assume the
test balloon has succeeded and that we can depend on on the presence of
<stdint.h>. Note that the [u]intptr_t types are optional and so we'd
need to make sure we avoid them in public interfaces.
[1] 7bc341e21b5 (git-compat-util: add a test balloon for C99 support,
2021-12-01)
It's not just the #includes, though. As stated in the original
document, we run into problems with platform-defined types and
everything else that's tweaked in/provided by git-compat-util.h:
- This header file that's included in the non-git projects can't use
`off_t` or `struct stat`.
In principle we could change the interfaces that currently use `off_t`
to use `int64_t` and convert to `off_t` in the function body which would
avoid having to have separate wrappers for the internal and external
callers. I'm not sure how invasive that would be though. `struct stat`
is trickier - where do we expose that in our interfaces?
- This header file can't assume that any types related to sockets are
available, because those come from <sys/socket.h> on Linux and from
winsock2.h on Windows.
- It can't assume that we have `NORETURN` (and it can't assume that we
don't need it), or `MAYBE_UNUSED`, or ...
These problems and the _GNU_SOURCE on you mention below must be pretty
common for cross-platform libraries - how do other projects handle them?
On the face of it this seems like it would be fairly simple to solve by
including a file that contains the subset of git-compat-util.h that
defines these macros (with a suitable LIBGIT_ prefix) in libgit.h.
Most of those issues _may_ be able to be resolved by having a
"gitlib-compat-util.h" file included at the top of the "external
project" .h file. But that's insufficient. Example:
#include <unistd.h>
#include "git/gitlib.h" // Oops, the `#define _GNU_SOURCE` in the
transitive "gitlib-compat-util.h" has no effect!
Or the opposite:
#include "git/gitlib.h" // Oops, this set _FILE_OFFSET_BITS=64 when
the project wasn't expecting it!
#include <unistd.h> // For this translation unit only, `off_t` might
be a different size than elsewhere in the project, I hope you like
debugging segfaults.
The only ways I could come up with to solve these problems were to
hold the "external interface" to a different standard, that is
simultaneously both more permissive (it can assume C99), and
restrictive (it can't rely on things like off_t),
>
incompatible with these external interfaces being used by the git
project itself, which has a broader set of platforms it needs to
support. But the external interfaces must be very simple wrappers
around code that _is_ shared with the git executable.
I agree we should minimize the amount of code in the wrappers for
external callers.
Best Wishes
Phillip
