From: Phillip Wood <phillip.wood123@gmail.com>
To: Kyle Lippincott <spectral@google.com>, Calvin Wan <calvinwan@google.com>
Cc: Git Mailing List <git@vger.kernel.org>,
"brian m. carlson" <sandals@crustytoothpaste.net>,
rsbecker@nexbridge.com, phillip.wood@dunelm.org.uk,
Josh Steadmon <steadmon@google.com>,
Emily Shaffer <nasamuffin@google.com>,
Enrico Mrass <emrass@google.com>
Subject: Re: [RFD] Libification proposal: separate internal and external interfaces
Date: Fri, 10 May 2024 10:52:14 +0100 [thread overview]
Message-ID: <12a4cd37-1cef-45a0-9b96-36a978e52dba@gmail.com> (raw)
In-Reply-To: <CAO_smVg_RmL2_ERXG0uZ+CgEHZBm34PvmNucbSTdsEDLjU-fhA@mail.gmail.com>
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
next prev parent reply other threads:[~2024-05-10 9:52 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-02 14:18 [RFD] Libification proposal: separate internal and external interfaces Calvin Wan
2024-04-07 21:33 ` brian m. carlson
2024-04-07 21:48 ` rsbecker
2024-04-08 1:09 ` brian m. carlson
2024-04-08 11:07 ` rsbecker
2024-04-08 21:29 ` Junio C Hamano
2024-04-09 0:35 ` brian m. carlson
2024-04-09 17:26 ` Calvin Wan
2024-04-09 9:40 ` Phillip Wood
2024-04-09 17:30 ` Calvin Wan
2024-04-22 16:26 ` Calvin Wan
2024-04-22 20:28 ` Junio C Hamano
2024-04-23 9:57 ` phillip.wood123
2024-05-09 1:00 ` Kyle Lippincott
2024-05-10 9:52 ` Phillip Wood [this message]
2024-05-10 21:35 ` Kyle Lippincott
2024-05-09 19:45 ` Kyle Lippincott
2024-05-09 20:14 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=12a4cd37-1cef-45a0-9b96-36a978e52dba@gmail.com \
--to=phillip.wood123@gmail.com \
--cc=calvinwan@google.com \
--cc=emrass@google.com \
--cc=git@vger.kernel.org \
--cc=nasamuffin@google.com \
--cc=phillip.wood@dunelm.org.uk \
--cc=rsbecker@nexbridge.com \
--cc=sandals@crustytoothpaste.net \
--cc=spectral@google.com \
--cc=steadmon@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).