On 10/29/12, Richard Biener <richard.guent...@gmail.com> wrote: > On Oct 29, 2012 Diego Novillo <dnovi...@google.com> wrote: > > On Oct 25, 2012 Lawrence Crowl <cr...@googlers.com> wrote: > > > The sbitmap non-bool returning bitwise operations have been > > > merged with the bool versions. Sometimes this merge involved > > > modifying the non-bool version to compute the bool value, > > > and sometimes modifying bool version to add additional work > > > from the non-bool version. The redundant routines have been > > > ifdef'd out. I will remove the ifdef'd out code later. > > > > No #if 0 code, please. Let's just remove them.
Okay, but I point out that there is an awful lot of #if 0 code out there. I would rather have done such removal in a followup patch. > > > The allocation functions have not been renamed, because we > > > often do not have an argument on which to overload. > > > > Would it work if we made the first argument a reference to the > > bitmap being allocated? I suppose this shouldn't be a big deal. > > It's definitely good to at least in one place see what kind of > bitmap is actually being used ... ;) The parameters to the allocation functions are different. So even if they had the same name, they are not interchangable. > > > The cardinality functions have not been renamed, because they > > > have different parameters, and are thus not interchangable. > > > > Why not change the parameters then? > > Agree, as a followup maybe. The sbitmap popcount function is only used in ebitmap, which is itself not used. If we do anything, removing them might be the thing to do. > > > The iteration functions have not been renamed, because they > > > are functionally different. > > > > Functionally different? How? The bitmap.h iterators set and the sbitmap.h iterator set intersect in one element. I would rather treat the iterator conversion as a separate patch. > > It seems like the patch only goes skin deep. I would rather > > make a true unification. If the only thing that remains the > > different are the allocation functions, that's not a big deal. > > But the point was to make everything else the same. > > Indeed. It's not quite skin deep as it removes the distinction between the value returning and non-value returning functions. The intent here is to change the skin. Richard said in an earlier mail, "I'd rather not mix this with any kind of further C++-ification (that is introduction of member functions or operator overloads)." So, exactly what are you all after? > > I also notice that the patch does not rename any of the SET_BIT, > > RESET_BIT functions in sbitmap.c. > > They should go - a followup is fine though. That was the intent. > > > Tested on x86_64. Config testing in progress. > > > > You will also want to test on a couple other architecture on > > the farm. By config testing, do you mean contrib/config-list.mk? Yes, I meant that. There are no functional changes here. Why is contrib/config-list.mk not sufficient? -- Lawrence Crowl
