Re: libgit2 - a true git library

[Pierre Habouzit <madcoder@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 4088 bytes --]

On Sat, Nov 01, 2008 at 02:26:45AM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Fri, 31 Oct 2008, Shawn O. Pearce wrote:
> 
> > Nicolas Pitre <nico@cam.org> wrote:
> > > On Fri, 31 Oct 2008, Shawn O. Pearce wrote:
> > > 
> > > > > Both the negative code and errno style are lightweight in the 
> > > > > common "no error" case.  The errno style is probably more handy 
> > > > > for those functions returning a pointer which should be NULL in 
> > > > > the error case.
> 
> Unfortunately, errno would not be thread-safe, unless you can guarantee 
> that errno is a thread-local variable.

Well, TLS afaict is implemented on arches that have GNU ld, or on win32
with a recent enough mingw. Though this is quite a requirement.

> > Oh, good point.  We could also stagger the errors so they are
> > always odd, and never return an odd-alignment pointer from a
> > successful function.  Thus IS_ERR can be written as:
> > 
> >   #define IS_ERR(ptr) (((intptr_t)(ptr)) & 1)
> > 
> > which is quite cheap, and given the (probably required anyway)
> > aligned allocation policy means we still have 2^31 possible
> > error codes available.
> 
> Oh boy, both solutions are ugly as hell.  Although the &1 method does not 
> limit the memory space as much (except if you plan to work in 
> space-contrained environments, where you do not want to be forced to 
> word-align structs).
> 
> The only pointer game I would remotely consider clean is if you had
> 
> 	const char *errors[] = {
> 		...
> 	};
> 
> 	inline int is_error(void *ptr) {
> 		return ptr >= errors && ptr < errors + ARRAY_SIZE(errors);
> 	}

Well, you can't return _sanely_ an error through a pointer. The &1
method is broken as soon as you return a char* (there is an alignment
requirement for malloc, not for any pointer out there), hence shall not
be used, as it would not be the sole way to test for error.

Another option, that is _theorically_ not portable, but is ttbomk on all
the platforms we intend to support (IOW POSIX-ish and windows), is to
use "small" values of the pointers for errors. [NULL .. (void *)(PAGE_SIZE - 1)[
cannot exist, which gives us probably always 512 different errors, and
the test is ((uintptr_t)ptr < (PAGE_SIZE)) which is cheap. It's butt
ugly, but encoding errors into pointers is butt ugly in the first place.


Another option that is what I would prefer, would be for the use of
errno where it makes sense. E.g. if you want a function that fetches an
object, this is somehow what read(3) would look like on our store, more
or less, and errno's are enough.  For the other functions where errno
cannot be used, I'm pretty sure we will always pass a handle to some
kind of libgit2 stuff, like the "repository" we're working on. The
_easiest_ way is to put the "last error" into that structure and use
that. I mean, if we want libgit2 to be useful for _everyone_ we *WILL*
have to pass a repository context around. I see almost no way around it.
And there, NULL means error, and if you want to know about the specific
error, git_repo_errno(&ctx) / git_repo_strerr(&ctx) is just easy.

Note: What is important is to be able to check for errors _fast_, I
don't think printing out the error and knowing which error it was would
be in the fast path, so it's less useful to have this information
immediately.


_My_ taste (but again, like the _t I would use what is there, and won't
make a fuss about it at all) is to see function return -1 or NULL for
errors, and "abuse" errno for system-like functions, or put the last
error into the context on which you're working (your "this" more or
less). We don't need a specific error context at all, because we already
have a repository context available.

I'm not really a fan of pointer semantics abuse (it's sometimes useful,
but as the public interface of a library, this is butt-ugly.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]