On Mon, Sep 30, 2024 at 11:35:50PM +0000, brian m. carlson wrote: > On 2024-09-30 at 22:44:37, Junio C Hamano wrote: > > "brian m. carlson" <sandals@xxxxxxxxxxxxxxxxxxxx> writes: > > > > > .... It is designed to be passed and returned by value, not > > > pointer, and it is possible to do so in two registers on 64-bit systems. > > > Similar functionality works well for error types in Rust and for the > > > standard library's lldiv_t, so this should not pose a problem. > > > > It should not, in the sense that "any reasonable platform should be > > able to pass two 64-bit word in a structure by value", but isn't it > > optimizing for a wrong (i.e. have error) case? In the case where > > there is no error, a "negative return is error, zero is success", > > with a pointer to "more detailed error info, in case the call > > resulted in an error", would let us take branch based on a zero-ness > > check on an integer in a machine-natural word, without even looking > > at these two words in the struct. > > We can adjust the first word so that it's always zero on success, in > which case, because it's returned in two registers, the processor will > be able to branch on a zero-ness check on one of those registers. (We > can even optimize the check by looking at the low 32 bits, which will do > the same for 32-bit machines as well.) The performance benefit will be > the same, and I should note that Rust does this kind of thing without a > problem. I was wondering the same here, also because having to write `git_error_ok()` is a bit unwieldy. One of my thoughts in this context was to not be shy of allocating the error structures such that we don't have to pass by value, but instead pass by pointer. It also gives us a bit more flexibility with the error structure itself, as we don't have to optimize for size anymore. Or at least not to the extent as we'd have to do with the current proposal. The obvious problem of course is if we're running out of memory. But I think we can easily special-case this and return a statically-allocated error specific to that situation, where `git_error_free()` would know not to free it. Patrick
