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

Equality visitor #2335

Closed
wants to merge 14 commits into from
Closed

Conversation

a-zakir
Copy link
Contributor

@a-zakir a-zakir commented Aug 16, 2024

draft

@a-zakir a-zakir changed the base branch from develop to feature/expression-visitors August 16, 2024 13:42
Copy link
Member

@flomnes flomnes left a comment

Choose a reason for hiding this comment

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

The key is to have something like

bool operator==(const Node& node1, const Node& node2)

that works for all node types. I'm not sure your implementation does that.

src/tests/src/solver/expressions/test_expressions.cpp Outdated Show resolved Hide resolved
src/tests/src/solver/expressions/test_expressions.cpp Outdated Show resolved Hide resolved
return ret.value();
}
}
logs.error() << "Antares::Solver::Nodes Visitor: unsupported Node!";
Copy link
Member

Choose a reason for hiding this comment

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

If node1 and node2 are of different types, we will encounter this line. So I think we should handle errors better here.
Example

Literal lit1(2), lit2(2);
AddNode node1(&lit1, &lit2);
MultiplicationNode node2(&lit1, &lit2);
EqualityVisitor equal;
// Here, we go through l. 78 above, but the caller only gets a log error
// and an `R()`, not so easy to catch
equal.dispatch(&node1, &node2); 

Copy link
Contributor Author

@a-zakir a-zakir Aug 19, 2024

Choose a reason for hiding this comment

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

update: the code print warning message and returns false

}
else
{
return {};
Copy link
Member

Choose a reason for hiding this comment

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

Different node types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the return type is handled in the dispatch method

@a-zakir a-zakir marked this pull request as ready for review August 19, 2024 10:11
@a-zakir a-zakir closed this Aug 20, 2024
@a-zakir a-zakir deleted the feature/EqualityVisitor branch September 4, 2024 14:40
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