On 8/13/19 6:51 PM, Aldy Hernandez wrote:
>> Presumably this was better than moving the implementation earlier.
>
> Actually, it was for ease of review. I made some changes to the
> function, and I didn't want the reviewer to miss them because I had
> moved the function wholesale. I can move the function earlier, after we
> agree on the changes (see below).
Either works for me. I think there was an informal effort to avoid
these kinds of forward decls eons ago because our inliner sucked, but in
the IPA world order in the source file really shouldn't matter.
>
>>
>> If we weren't on a path to kill VRP I'd probably suggest a distinct
>> effort to constify this code. Some of the changes were a bit confusing
>> when it looked like we'd dropped a call to set the range of an object.
>> But those were just local copies, so setting the type/min/max directly
>> was actually fine. constification would make this a bit clearer. But
>> again, I don't think it's worth the effort given the long term
>> trajectory for tree-vrp.c.
>
> I shouldn't be introducing any new confusion. Did I add any new methods
> that should've been const that aren't? I can't see any??. I'm happy to
> fix anything I introduced.
IIRC we had an incoming range object passed by value, which we locally
modified and called the setter.
I spotted the dropped call to the setter and was going to call it out as
possibly broken. But in investigating further I realized the object was
passed by value, so dropping the setter wasn't really a problem.
THe funny thing was we were doing this on source operands rather than
the destination operand. Arguably the ranges for the source operands
should be constant which would have flagged that code as fishy from its
inception and I'm sure the code would have been restructured
appropriately and would have avoided the confusion.
So in summary, you didn't break anything. It was a safe change you
made, but it wasn't immediately obvious it was safe. If we had a
constified codebase the intent of the code would have been more obvious.
>
>>
>>
>> So where does the handle_pointers stuff matter? I'm a bit surprised we
>> have to do anything special for them.
>
> I've learned to touch as little of VRP as is necessary, as changing
> anything to be more consistent breaks things in unexpected ways ;-).
>
> In this particular case, TYPE_MIN_VALUE and TYPE_MAX_VALUE are not
> defined for pointers, and I didn't want to change the meaning of
> vrp_val_{min,max} throughout. I was trying to minimize the changes to
> existing behavior. If it bothers you too much, we could remove it as a
> follow up when we are sure there are no expected side-effects from the
> rest of the patch. ??
I don't mind exploring this as a follow-up. I guess that a min/max
doesn't really have significant meaning for pointers.
I think rather than digging too deep into this, let's table it for now.
I think the time to revisit will be as we work through removal of
tree-vrp at some point in the future.
>
>>
>>
>> OK. I don't expect the answers to the minor questions above will
>> ultimately change anything.
>
> I could appreciate a final nod before I commit. And even then, I will
> wait until the other patch is approved and commit them simultaneously.
> They are a bit intertwined.
I'm nodding :-)
jeff
