On Tue, Nov 5, 2019 at 2:15 PM Aldy Hernandez <al...@redhat.com> wrote: > > The function range_int_cst_p only works with VR_RANGE's at the moment. > This is silly because VR_ANTI_RANGE and even VR_VARYING can contain > numeric bounds. I have fixed this oversight and have made the function > return the bounds in MIN/MAX. This simplifies a lot of code, because > there is no longer a need to special case VR_VARYING and VR_ANTI_RANGE, > as well as pick at the individual range components outside of the API. > > The patch has the pleasant side-effect of bringing more things into the > API fold. Basically, any access to either value_range::min(), max(), or > kind(), is suspect and a big hint that the code should be rewritten to > use the API (contains_p, varying_p, zero_p, etc). > > One of the primary culprits of API noncompliance is the sprintf and > strlen warning code. Mind you, not due to negligence on the author, but > because we had no value-range API when Martin added the passes. I > realize it's nobody's responsibility to fix older value-range code, and > I'll probably end up doing it myself (next cycle??), but I could > definitely use a hand from the experts, as it's intricate and delicate code. > > Speak of which, in converting dump_strlen_info() to use the new > range_int_cst_p, I noticed a lot of the code disappeared if we used the > API. Martin, if you'd prefer not to dump varying, undefined, etc, let > me know and we can gate that call to vr.dump(). I took the liberty > because it was simple, clean, and hidden away in an internal debugging > helper. > > OK for trunk?
No. It's a semantic change, no? Don't we for VR_ANTI_RANGE always get [-INF, +INF] back then? Likewise for VARYING? What do we get for UNDEFINED? I think callers are not prepared for this and expect it to return true for "useful" ranges only. If you want this, use a new name, like get_range_bounds (). Also not sure why min/max need to be INTEGER_CST, why not _always_ return something (that is, the fucntion should never need to return false). The patch doesn't look like an improvement, it just adds to confusion. Richard.
