Re: [PATCH 2/7] move setting of object->type to alloc_* functions
- From
Ramsay Jones <ramsay@ramsay1.demon.co.uk>
- Date
- Jul 13, 2014, 19:27 UTC
- Message-ID
- <53C2DDB7.2070708@ramsay1.demon.co.uk>
- In-Reply-To
- <20140713064116.GA4768@sigill.intra.peff.net>
On 13/07/14 07:41, Jeff King wrote:
Show 15 quoted lines
> On Sat, Jul 12, 2014 at 02:05:39PM -0400, Jeff King wrote: > >>> I don't particularly like 'flag' here. (not a massive dislike, mind you:) >>> >>> Perhaps: flag->object_type, type->node_type? >>> Or, if that's too verbose, maybe just: flag->type, type->node? >> >> Me either, but as you noticed, type was taken. Your suggestions seem >> fine. We could also just do away with the macro as discussed earlier (we >> already do in the commit_node case, anyway...). > > Thinking on this more, writing out the definitions is the only sane > thing to do here, now that alloc_commit_node does not use the macro. > Otherwise you are inviting people to modify the macro, but fail to > notice that the commit allocator also needs updating.
Hmm, well I could argue that using the macro for all allocators, apart from alloc_commit_node(), clearly shows which allocator is the odd-man out (and conversely, that all others are the same)! :-P
No, I don't think this is a telling advantage; I don't think it makes that much difference. (six of one, half-a-dozen of the other).
BTW, I tested the previous series on Linux 32-bit, Cygwin 32-bit, MinGW 32-bit and Cygwin 64-bit. (I can't test on Linux 64-bit, since I can't get Linux installed on my new laptop :( ). Admittedly, the testing on MinGW and Cygwin was only fairly light (it takes *hours* to run the full testsuite, and I just don't have the time).
I was slightly concerned, when reading through this new series, that the alloc_node() function may no longer be inlined in the new allocators. However, I have just tested on Linux (only using gcc this time), and it was just fine. I will test the new series on the above systems later (probably tomorrow) but don't expect to find any problems.
> > Here's a re-roll. The interesting bit is the addition of the second > patch (but the rest needed to be rebased on top).
Yep, this looks good. Thanks!
Show 9 quoted lines
> > [1/8]: alloc.c: remove the alloc_raw_commit_node() function > [2/8]: alloc: write out allocator definitions > [3/8]: move setting of object->type to alloc_* functions > [4/8]: parse_object_buffer: do not set object type > [5/8]: add object_as_type helper for casting objects > [6/8]: alloc: factor out commit index > [7/8]: object_as_type: set commit index > [8/8]: diff-tree: avoid lookup_unknown_object
ATB, Ramsay Jones