Skip to content

Python: Add scope entry definition nodes #15166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 20, 2023

Conversation

yoff
Copy link
Contributor

@yoff yoff commented Dec 19, 2023

otherwise we confuse captured variables
in the single scope entry cfg node. Now
we have one for each defined variable.

@yoff yoff force-pushed the python/add-scope-entry-definition-nodes branch from 0a57ee3 to c7d0dab Compare December 20, 2023 09:55
yoff added 3 commits December 20, 2023 12:08
otherwise we confuse captured variables
in the single scope entry cfg node. Now
we have one for each defined variable.
also, it is slightly incorrect...
@yoff yoff force-pushed the python/add-scope-entry-definition-nodes branch from c7d0dab to 07c88dc Compare December 20, 2023 11:09
@yoff yoff marked this pull request as ready for review December 20, 2023 11:10
@yoff yoff requested a review from a team as a code owner December 20, 2023 11:10
@yoff
Copy link
Contributor Author

yoff commented Dec 20, 2023

Evaluation came back as an optimization 🎉

@yoff yoff merged commit 19813c8 into github:main Dec 20, 2023
@@ -484,8 +484,7 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
* or at runtime when callables in the module are called.
*/
predicate simpleLocalFlowStepForTypetracking(Node nodeFrom, Node nodeTo) {
IncludePostUpdateFlow<PhaseDependentFlow<LocalFlow::localFlowStep/2>::step/2>::step(nodeFrom,
Copy link
Member

Choose a reason for hiding this comment

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

Can you give some more details? Ah, from looking over the implementation of LocalFlow::localFlowStep (src) that already does the PhaseDependentFlow + IncludePostUpdateFlow stuff 👍

@RasmusWL
Copy link
Member

RasmusWL commented Jan 3, 2024

@yoff can you please highlight why the commit message for 07c88dc says also, it is slightly incorrect...? What part is slightly incorrect now? 😕

@yoff
Copy link
Contributor Author

yoff commented Jan 8, 2024

@yoff can you please highlight why the commit message for 07c88dc says also, it is slightly incorrect...? What part is slightly incorrect now? 😕

The PR description starts "remove unnecessary post-processing" and it is this post-processing that was slightly incorrect (in addition to being unnecessary). As it has been removed, there are no slightly incorrect parts left :-) (🤞)

@RasmusWL
Copy link
Member

RasmusWL commented Jan 9, 2024

Ah, usually the old version is past-tense, so it would be more natural to say "also, it was slightly incorrect" -- at least that would have made me understand straight away :)

@yoff
Copy link
Contributor Author

yoff commented Jan 10, 2024

Ah, usually the old version is past-tense, so it would be more natural to say "also, it was slightly incorrect" -- at least that would have made me understand straight away :)

Good point, I will keep that convention in mind going forward 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants