-
Notifications
You must be signed in to change notification settings - Fork 738
(refactor): Updating analysis framework to be easier to use #9564
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
(refactor): Updating analysis framework to be easier to use #9564
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06f2a64601
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
orizi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orizi reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @eytan-starkware and @TomerStarkware).
eytan-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eytan-starkware made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @TomerStarkware).
06f2a64 to
024ddbf
Compare
eytan-starkware
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eytan-starkware resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @TomerStarkware).
024ddbf to
2ad86bb
Compare
2ad86bb to
32bfb79
Compare
orizi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@orizi reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @TomerStarkware).

Summary
Added a
block_entry_locationmethod to theDataflowAnalyzertrait to determine the appropriate location for block entry based on analysis direction. This method handles both backward and forward analysis cases, recursively finding locations when needed. Also modified themergemethod signature to take theloweredparameter instead of a separatelocationparameter, and updated theEdgeenum to use owned rather than referenced values for certain fields. Added alocationmethod toBlockEndto extract location information when available.Type of change
Please check one:
Why is this change needed?
This change improves the dataflow analysis framework by providing a consistent way to determine block entry locations based on analysis direction. It simplifies the API by having the analyzer determine the appropriate location rather than requiring it to be passed separately, and makes ownership clearer by using owned values where appropriate.
What was the behavior or documentation before?
Previously, the
mergemethod required a separate location parameter, and there was no standardized way to determine block entry locations based on analysis direction.What is the behavior or documentation after?
Now the
DataflowAnalyzertrait includes ablock_entry_locationmethod that handles both backward and forward analysis cases, recursively finding locations when needed. Themergemethod signature has been updated to take theloweredparameter instead of a separatelocationparameter.