Improve style in goto_program_template.h #859
Conversation
| targett target, | ||
| targetst &successors) | ||
| template <typename Target> | ||
| std::list<Target> goto_program_templatet<codeT, guardT>::get_successors( |
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.
| if(target==instructions.end()) | ||
| return; | ||
| return std::list<Target>(); |
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. |
|
|
||
| if(!i.guard.is_true() && next!=instructions.end()) | ||
| if(i.guard.is_false() && next!=instructions.end()) |
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.
| const instructiont &i=*target; | ||
|
|
||
| if(i.is_goto()) | ||
| if(i.is_assume() && i.guard.is_true() && next!=instructions.end()) |
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
| const auto op=std::bind1st( | ||
| std::mem_fun(static_cast<ftype>(&goto_programt::insert_before)), &body); | ||
| return insert_preserving_source_location(pos, op); | ||
| return insert_preserving_source_location( |
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.
|
|
||
| if(i.is_goto()) | ||
| if(i.is_assume() && !i.guard.is_false() && next!=instructions.end()) |
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.
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.