Improve style in goto_program_template.h#859
Conversation
There was a problem hiding this comment.
This returns a list, which should either use move semantics (fast), or be constructed directly in the calling scope via RVO (really fast), so I doubt there's any penalty to this vs. modifying a list through a mutable reference.
There was a problem hiding this comment.
By using early returns, we can restrict the scope of mutable state, which makes it easier to reason about the behaviour of this method.
|
I'm aware of the failing tests - I'll fix this up when I get an opportunity. |
There was a problem hiding this comment.
This is wrong - a guard is an expression an can be true, false, or something non-constant. Thus !i.guard.is_true() is not the same as i.guard.is_false(). That might well fix failing tests.
There was a problem hiding this comment.
See above, this is wrong.
reuk
force-pushed
the
goto-prog-style
branch
2 times, most recently
from
c9066ff to
007a911
Compare
There was a problem hiding this comment.
This was broken due to insert_before changing to take a const_targett instead of a targett. I could just fix the typedef, but Meyers recommends in 'Effective Modern C++' that we prefer lambdas over std::bind, so I've made that change too.
There was a problem hiding this comment.
You must not conflate i.is_assume() and the remainder of the test into one condition as this would yield a single-entry list either way.
There was a problem hiding this comment.
Good catch! Fixed now.
The biggest change is to `goto_program_templatet::get_successors`, which now returns a list of targets instead of modifying a list which was passed in. With move semantics, there should be no performance penalty. It also cleans up call sites of this method, which no longer need to have a preceding list declaration. This change was made because there were two version of `get_successors`, one `const` and one not, with slightly different implementations. By templating the method on the `Target` type, we can reduce duplication, and avoid bugs where const- and non-const-methods have different behaviours. Also replaced a few iterator increments/decrements with `std::next` and `std::prev`, and made some const-correctness fixes.
3 participants
The biggest change is to
goto_program_templatet::get_successors, which now returns a list of targets instead of modifying a list which was passed-by-reference. With move semantics, there should be no performance penalty. It also cleans up call sites of this method, which no longer need to have a preceding list declaration.This change was made because there were two version of
get_successors, oneconstand one not, with slightly different implementations. By templating the method on theTargettype, we can reduce duplication, and avoid bugs where const- and non-const-methods have different behaviours.Also replaced a few iterator increments/decrements with
std::nextandstd::prev, and made some const-correctness fixes.