-
Notifications
You must be signed in to change notification settings - Fork 56
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
Fix compile errors on clang and Power #129
Conversation
…ncompile.cpp about total_daddy variable, disabling
@@ -385,8 +385,7 @@ bool improveGraph(NGHolder &g, som_type som) { | |||
|
|||
const vector<NFAVertex> ordering = getTopoOrdering(g); | |||
|
|||
return enlargeCyclicCR(g, som, ordering) | |||
| enlargeCyclicCR_rev(g, ordering); | |||
return enlargeCyclicCR(g, som, ordering) || enlargeCyclicCR_rev(g, ordering); |
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.
This is not equivalent, FYI. Graph g can be changed in both directions with |
rather than ||
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.
clang 14 complains if you use bitwise OR on booleans, hence the change.
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.
unless you mean for || the compiler only has to evaluate the first to be true for optimization, so the second may not be executed. In which case, I will probably have to evaluate both separately and just OR the results.
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.
Yes, I think that's what @danlark1 meant: both sides may need to be evaluated for their side effects, so switching from |
to ||
could be incorrect.
It would be safer to rewrite this instead like you mentioned, ORing the results:
return enlargeCyclicCR(g, som, ordering) || enlargeCyclicCR_rev(g, ordering); | |
bool lhs = enlargeCyclicCR(g, som, ordering); | |
bool rhs = enlargeCyclicCR_rev(g, ordering); | |
return lhs || rhs; |
No description provided.