Re: [1/4] [patch, middle-end] Make mpz_set/get_double_int functions non-static.

[Brooks Moses <brooks dot moses at codesourcery dot com>]

At 06:04 PM 4/6/2007, Zdenek Dvorak wrote:
> > I'm not entirely certain that this is the best way this handles
> > get_type_bounds, though it's the best I could think of.  This is
> > currently a static function that returns the upper and lower bounds of
> > an integer type, in mpz_t form -- or, more accurately, what it returns
> > are the maximum and minimum representable values for an unsigned integer
> > or a two's-complement signed integer using the full bitfield used by the
> > type; there is of course no guarantee that these are in fact within the
> > legal range of the type.  It's used in mpz_to_double_int to determine if
> > a value is in range or not; it's also used elsewhere in
> > tree-ssa-loop-niter.c.  Because of the caveats about it not really
> > returning the upper and lower bounds, I don't think it's a useful
> > function to export generally, so I simply duplicated it as a static
> > function in double-int.c and left the original in tree-ssa-loop-niter.c.
>
> now, this looks wrong to me.  I do not really want to argue whether it is
> "useful function to export generally", however, obviously you need the
> functionality at at least two places.  If you prefer, change the name of
> the function/improve the comments to reflect your doubts (i.e., that it
> does not really return bounds of the type, but rather minimum and
> maximum value of the realization of the type), but just cloning the
> function whenever you need it somewhere is IMHO not a good idea.

Ok, fair enough.  Do you have any suggestions for where this particular 
function should go, if it's exported?  That was one of the reasons I did it 
this way -- I couldn't find anything that seemed like a natural place for 
it, and was hoping that if someone objected to the duplication, they'd have 
a better suggestion.  :)

(I suppose if nobody suggests a better alternative, I could put it in 
tree.c with the other type-bounds functions.  It's rather unique in 
returning mpz_t representations rather than double_ints or trees, though.)

One other reason for the duplication -- because of the caveats I mentioned, 
I was expecting a possibility of making followup patches to the 
get_type_bounds code here, and I didn't expect that any changes would 
necessarily be correct for the  tree-ssa-loop-niter.c version.  Thus, I was 
expecting a fair chance that the two static functions would diverge, such 
that it wouldn't be duplicate code/functionality after all.

In particular, now that I've thought about it some more, I'm pretty sure 
that using TYPE_MIN_VALUE and TYPE_MAX_VALUE here is the right thing to do 
for mpz_get_double_int, which means that it shouldn't use this 
get_type_bounds code at all.  I'm not sure if it's clearer to leave this 
patch as it is (which is just a straight-across copy with no algorithmic 
changes) and submit a followup patch to change the algorithm and eliminate 
the new copy of get_type_bounds, or to combine everything into one patch 
that moves and changes things.  My personal preference is for the former, 
but I can do the latter if that's preferred.

> ...
> > +  if (!wrap)
> > +    {
> > +      gcc_assert (count <= 2);
> > +      mpz_clear (min);
> > +      mpz_clear (max);
> > +    }
>
> Although this is correct, calling mpz_clear already inside the first if
> seems more natural.

A good point.  And the declaration of min and max should go in there, 
too.  I'll change that before resubmitting.

Thanks for the comments!
- Brooks