Skip to content

Conversation

yoff
Copy link
Contributor

@yoff yoff commented Nov 23, 2020

Tests showing that we do not handle variable capture (ported from C#, adapted slightly).

@yoff yoff requested a review from a team as a code owner November 23, 2020 15:48
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

Overall LGTM, nice to have test of this 👍

I would really like a comment (in the code) explaining why it is important to have BOTH Out and Through functions. From your explanation that variable-capture might end up depending on the call-graph, it makes sense to test both scenarios, but when I forget about this in 6 months time, I will certainly be confused about this subtle nuance.

Nitpick: what is up with calling the functions Out and not out? 😄

yoff and others added 2 commits November 24, 2020 14:11
Co-authored-by: Rasmus Wriedt Larsen <rasmuswriedtlarsen@gmail.com>
@yoff yoff requested a review from RasmusWL November 24, 2020 13:14
Copy link
Member

@RasmusWL RasmusWL left a comment

Choose a reason for hiding this comment

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

Discussed in person that although I was not 100% convinced by the added comments explaining why we need both out and through, we would just move on 🚋 :shipit:

@RasmusWL RasmusWL merged commit aa4345a into github:main Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants