Skip to content
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

Implement refinements for acyclic regions #179

Merged
merged 14 commits into from
Jul 16, 2018

Conversation

HMPerson1
Copy link
Collaborator

@HMPerson1 HMPerson1 commented Jul 6, 2018

  • Replace nodes while maintaining topological order
  • Condition-based refinements
    • if-then-else
    • all if-thens
    • Test case
  • Condition-aware refinements Postponed until Fold conditional flags into original ones #171 is done
  • Reachability-based refinements
    • Test case
      • More aggressive condition simplifications

@XVilka
Copy link
Contributor

XVilka commented Jul 12, 2018

@sushant94 @chinmaydd @kriw please review

@chinmaydd
Copy link
Contributor

I think @HMPerson1 is yet to complete 2 TODOs (or he can split them into another PR).

Each condition now also stores its own negation so we can easily find the
negation of an expression inside a different expression and simplify based on
that.
@HMPerson1 HMPerson1 changed the title [WIP] Implement refinements for acyclic regions Implement refinements for acyclic regions Jul 13, 2018
@HMPerson1
Copy link
Collaborator Author

@sushant94 This can be merged.

@chinmaydd
Copy link
Contributor

@kriw would you like to take a look at this PR and help review it?

@@ -0,0 +1,32 @@
#[derive(Debug, Eq, PartialEq)]
pub enum AstNode<B, C, V> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would like you to leave comments what each B, C, V represents.

Cond(c, t, oe) => {
let ot = simplify_ast_node::<A>(cctx, *t);
let oe = oe.and_then(|e| simplify_ast_node::<A>(cctx, *e));
if c.is_true() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

   match (c, t, oe) {
        (c, _, _) if c.is_true() => ot,
        (c, _, _) if c.is_false() => oe,
        (c, Some(t), _) => Some(...),
        (c, _, Some(e), _) => Some(...),
        _ => None
    }

would shorten the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The c.is_true/false can't be in a match because by-move patterns can't have pattern guards, but the last part is indeed cleaner. Thanks!

@kriw
Copy link
Collaborator

kriw commented Jul 15, 2018

LGTM :)

@sushant94
Copy link
Collaborator

Merging. Thanks all!

@sushant94 sushant94 merged commit 27e21f9 into radareorg:master Jul 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants