Re: libgit2 - a true git library

From: Pierre Habouzit <madcoder@debian.org>
Date: Nov 1, 2008, 11:01 UTC
Message-ID: <20081101110120.GA3819@artemis.corp>
In-Reply-To: <alpine.DEB.1.00.0811010320370.22125@pacific.mpi-cbg.de.mpi-cbg.de>

On Sat, Nov 01, 2008 at 02:26:45AM +0000, Johannes Schindelin wrote:

Show 14 quoted lines

> 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.

Show 24 quoted lines

> > 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