On 8/14/19 1:53 PM, Jeff Law wrote:
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.
Ok, I'll leave it as is, so I don't have to rebase the VARYING patch.
When both patches are in, I'll move the definition up as an obvious change.
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 :-)
I've tested this patch in isolation, and am committing it while we agree
on the varying one.
Thank you.
Aldy
