Skip to content
This repository was archived by the owner on Jan 5, 2023. It is now read-only.

Conversation

@max-schaefer
Copy link
Contributor

Currently, the entry node is required to be a post-update SSA node, so we get an entry node for the result of NewEncoder in this snippet:

enc := json.NewEncoder(w)
enc.Encode(data)

But not in this one:

json.NewEncoder(w).Encode(data)

This is unfortunate, since in this case it prevents us from stitching together the models for Encode (first argument -> receiver) and NewEncoder (result -> first argument).

This PR proposes to fix this by taking the entry node to be the result itself if it isn't assigned to an SSA variable. (We still do want it to be the SSA variable if there is one, since otherwise we'd lose flow in the first example.)

It feels like there should be a more principled solution, but for now I think this fixes our immediate problem.

For ease of reviewing, I have structured this PR into three commits:

  • First commit just adds a test; result changes can be reviewed lightly, since the implementation didn't change.
  • Second commit contains surgical change to just fix the problem above; result changes are small and hopefully show that the intended effect is achieved.
  • Third commit slightly generalises the fix.

@max-schaefer
Copy link
Contributor Author

Initial evaluation on https://github.com/cockroachdb/cockroach looks promising, but I'm running a full comparison to be on the safe side.

@smowton
Copy link
Contributor

smowton commented Sep 22, 2020

Checking my understanding of the generalisation: we stop caring at all about PostUpdateNodes here because this isn't really updating anything, it's an output that so happens to behave quite similarly to post/pre-update node association when dealing with SSA def-use chains, but not in the direct use case?

@max-schaefer
Copy link
Contributor Author

Yes, I think that's a fair summary.

Copy link
Contributor

@smowton smowton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Righto, in that case I approve this message.

@max-schaefer max-schaefer marked this pull request as ready for review September 23, 2020 08:14
@max-schaefer
Copy link
Contributor Author

Evaluation came back fine.

@max-schaefer max-schaefer merged commit 6130720 into github:main Sep 23, 2020
@jason-invision
Copy link
Contributor

It worked. I was able to remove my work around and get the correct results.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants