Martin Sebor msebor@gmail.com
Tue Jun 11 23:57:00 GMT 2019

```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?
>>>>
>>>>
>>>> +Â  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

```