This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]