Add value_range_base::contains_p

Martin Sebor msebor@gmail.com
Tue Jun 11 16:52:00 GMT 2019


On 6/11/19 10:26 AM, Aldy Hernandez wrote:
> 
> 
> On 6/11/19 12:17 PM, Martin Sebor wrote:
>> On 6/11/19 9:05 AM, Aldy Hernandez wrote:
>>>
>>>
>>> On 6/11/19 9:45 AM, Richard Biener wrote:
>>>> On Tue, Jun 11, 2019 at 12:40 PM Aldy Hernandez <aldyh@redhat.com> 
>>>> wrote:
>>>>>
>>>>> This patch cleans up the various contains, may_contain, and
>>>>> value_inside_range variants we have throughout, in favor of one--
>>>>> contains_p.  There should be no changes in functionality.
>>>>>
>>>>> I have added a note to range_includes_zero_p, perhaps as a personal
>>>>> question than anything else.  This function was/is returning true for
>>>>> UNDEFINED.  From a semantic sense, that doesn't make sense.  UNDEFINED
>>>>> is really the empty set.  Is the functionality wrong, or should we 
>>>>> call
>>>>> this function something else?  Either way, I'm fine removing the 
>>>>> comment
>>>>> but I'm genuinely curious.
>>>>
>>>> So this affects for example this hunk:
>>>>
>>>> -      if (vr && !range_includes_p (vr, 1))
>>>> +      if (vr && (!vr->contains_p (build_one_cst (TREE_TYPE (name)))
>>>> +                && !vr->undefined_p ()))
>>>>          {
>>>>
>>>> I think it's arbitrary how we compute X in UNDEFINED and I'm fine
>>>> with changing the affected predicates to return false.  This means
>>>> not testing for !vr->undefined_p here.
>>>
>>> Excellent.
>>>
>>>>
>>>> Note I very much dislike the build_one_cst () call here so please
>>>> provide an overload hiding this.
>>>
>>> Good idea.  I love it.
>>>
>>>>
>>>> Not sure why you keep range_includes_zero_p.
>>>
>>> I wasn't sure if there was some subtle reason why we were including 
>>> undefined.
>>>
>>> OK pending tests?
>>
>> Should the second overload:
>>
>> +  bool contains_p (tree) const;
>> +  bool contains_p (int) const;
>>
>> take something like HOST_WIDE_INT or even one of those poly_ints
>> like build_int_cst does?  (With the former, contains_p (0) will
>> likely be ambiguous since 0 is int and HOST_WIDE_INT is long).
> 
> We have a type, so there should be no confusion:
> 
> +  return contains_p (build_int_cst (type (), val));
> 
> (UNDEFINED and VARYING don't have a type, so they are special cased prior).

I didn't mean the overloads are confusing, just that there the one
that takes an int doesn't make it possible to test whether a value
outside the range of an int is in the range.  For example, in
the call

   contains_p (SIZE_MAX)

the argument will get sliced (and trigger a -Woverflow).  One will
need to go back to the more verbose way of calling it.

Changing the argument type to HOST_WIDE_INT would avoid the slicing
and warning but then make contains_p (0) ambiguous because 0 isn't
a perfect match for either void* or long (so it requires a conversion).

Martin

> 
> Aldy



More information about the Gcc-patches mailing list