Add value_range_base::contains_p

[Aldy Hernandez]

On 6/11/19 12:52 PM, Martin Sebor wrote:
> 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.

The int version is not really meant to pass anything but simple 
constants.  For anything fancy, one should really be using the tree 
version.  But I can certainly change the argument to HOST_WIDE_INT if 
preferred.

> 
> 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).

Just a plain 0 will match the int version, instead of the tree version, 
right?  Nobody should be passing NULL to the tree version, so that seems 
like a non-issue.

Aldy