Add value_range_base::contains_p

[Martin Sebor]

On 6/11/19 3:02 PM, Aldy Hernandez wrote:
> 
> 
> 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.

Right, NULL isn't a problem, but I would expect any integer to work
(I thought that's what Richard was asking for)  So my suggestion was
to have contains_p() a poly_int64 and provide just as robust an API
as build_int_cst.  The argument ends up converted to the poly_int64
anyway when it's passed to the latter.  I.e., why not define
contains_p simply like this:

   bool
   value_range_base::contains_p (poly_int64 val) const
   {
     if (varying_p ())
       return true;
     if (undefined_p ())
       return false;

     return contains_p (build_int_cst (type (), val));
   }

Martin