Re: [PATCH 0/7] ensure index is set for all OBJ_COMMIT objects variable
On 11/07/14 09:41, Jeff King wrote: > Here's a series to address the bug I mentioned earlier by catching the > conversion of OBJ_NONE to OBJ_COMMIT in a central location and setting > the index there. > > I've included your patch 1/2 unchanged in the beginning, as I build on > top of it (and your patch 2/2 is no longer applicable). The rest is > refactoring leading up to patch 6 to fix the bug. Patch 7 is a bonus > cleanup. I have just read this series in my email client (I will apply and test them later), but this looks very good to me. :) Only one patch gave me slight pause; see later. > > I'd hoped to cap off the series by converting the "type" field of > "struct commit" to a "const unsigned type : 3", which would avoid any > new callers being added that would touch it without going through the > proper procedure. However, it's a bitfield, which makes it hard to cast > the constness away in the actual setter function. My best attempt was to > use a union with matching const and non-const members, but that would > mean changing all of the sites which read the field (and there are many) > to use "object->type.read". > > There may be a clever solution hiding in a dark corner of C, but I > suspect we are entering a realm of portability problems with older > compilers (I even saw one compiler's documentation claim that "const" > was forbidden on bitfields, even though C99 has an example which does > it). Yes, I've come across such compilers too; I wouldn't go there! ;-P ATB, Ramsay Jones -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/7] ensure index is set for all OBJ_COMMIT objects variable
Here's a series to address the bug I mentioned earlier by catching the conversion of OBJ_NONE to OBJ_COMMIT in a central location and setting the index there. I've included your patch 1/2 unchanged in the beginning, as I build on top of it (and your patch 2/2 is no longer applicable). The rest is refactoring leading up to patch 6 to fix the bug. Patch 7 is a bonus cleanup. I'd hoped to cap off the series by converting the "type" field of "struct commit" to a "const unsigned type : 3", which would avoid any new callers being added that would touch it without going through the proper procedure. However, it's a bitfield, which makes it hard to cast the constness away in the actual setter function. My best attempt was to use a union with matching const and non-const members, but that would mean changing all of the sites which read the field (and there are many) to use "object->type.read". There may be a clever solution hiding in a dark corner of C, but I suspect we are entering a realm of portability problems with older compilers (I even saw one compiler's documentation claim that "const" was forbidden on bitfields, even though C99 has an example which does it). [1/7]: alloc.c: remove the alloc_raw_commit_node() function [2/7]: move setting of object->type to alloc_* functions [3/7]: parse_object_buffer: do not set object type [4/7]: add object_as_type helper for casting objects [5/7]: alloc: factor out commit index [6/7]: object_as_type: set commit index [7/7]: diff-tree: avoid lookup_unknown_object -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
