This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [patch] Unify bitmap interface.
On 10/29/12, Richard Biener <richard.guenther@gmail.com> wrote:
> On Oct 29, 2012 Diego Novillo <dnovillo@google.com> wrote:
> > On Oct 25, 2012 Lawrence Crowl <crowl@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