-
Notifications
You must be signed in to change notification settings - Fork 77
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
Audit usages of fold_right
#1688
base: master
Are you sure you want to change the base?
Conversation
| None -> None | ||
| Some st -> | ||
let reachable acc e = | ||
Option.bind acc (fun st -> |
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 could even use let-punning after let open GobOption.Syntax in
or something.
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.
I played around with it, but was unconvinced whether it is easier / more beautiful:
let reachable acc e =
let open GobOption.Syntax in
let* st = acc in
let ad = ask.f (Queries.ReachableFrom e) in
if Queries.AD.is_top ad then
None
else
Some (Queries.AD.join ad st)
in
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.
@jerhard @DrMichaelPetter @karoliineh @vesalvojdani Opinions on monadic syntax or not?
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.
Me as a contributor more on the novice side would rather see the explicit binding version instead of the let-punning - call me a simpleton, but I like it better the explicit way.
This PR replaces usages of
fold_right
with usages offold_left
wherever there is obviously no difference between both. This also raised an instances where I am unsure if the code in Goblint is correct.