-
Notifications
You must be signed in to change notification settings - Fork 276
Implemented a visitor class for codet #1789
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
Conversation
d6d2b09
to
56d0949
Compare
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.
Seems reasonable, couple of nitpicks
src/util/code_visitor.h
Outdated
/// \param id: expr: expression | ||
void operator()(exprt &expr) override | ||
{ | ||
if(expr.id() != ID_code || !expr.is_not_nil()) |
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.
Condition is redundant -- nil expressions have id()
== ""
src/util/code_visitor.h
Outdated
cmapt call_map; | ||
}; | ||
|
||
/// \brief codet visitor, leverages expression visitor to visit all the codet's |
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.
Language nitpick: s/leverages/uses/
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.
What template/documented idea of "visitor" is this following? It neither uses Gang-of-Four naming conventions nor are (the equivalents of) visit() calls very efficient (as they require going through a std::map).
Here is what needs to happen:
- If it's your own design, use your own terminology and clearly steer away from existing one.
- If it isn't a completely novel design, reference your source.
- Tests are strictly required.
- Explain when and when not to use this functionality.
@tautschnig regarding "Explain when and when not to use this functionality" should this be manifested as comments in the header file? |
@tautschnig This is not a classic visitor pattern. As This would be a bit of pain to implement properly, at least for codet, it extends the behaviour of the existing expression visitor. Pattern wise, it is inspired from the observable-pattern, with the difference that observers are registered through a callback instead of inheriting from some IObserve (Makes things simpler for our purposes). Agreed the terminology I used is bit confusing, I will update the code to make it clearer, I will also add tests. |
I think comments in the code are probably best. The main reason this comment of mine even came up is that you are introducing new functionality that isn't used anywhere. It's a new feature that isn't user-visible, so you need to tell developers what to do. |
56d0949
to
5e14b9c
Compare
@tautschnig , latest commit should address the issues you raised, can you confirm please? (tests incoming) |
5e14b9c
to
65cbc1e
Compare
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.
A unit test is required as bare minimum. Various other fixes as indicated in comments.
src/util/code_observer.h
Outdated
{ | ||
static_assert(std::is_base_of<codet, T>::value, "T is not derived from codet"); | ||
public: | ||
virtual ~code_observer_baset() {} |
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.
You might use =default;
?
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.
done, assumed you meant " = default; " (new formating rules)
src/util/code_observer.h
Outdated
public: | ||
virtual ~code_observer_baset() {} | ||
|
||
/// registers an observer, that is associated with specific codet id. |
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.
Nit pick: no comma before "that" (as a general rule) - also applies elsewhere in this PR.
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.
done
src/util/code_observer.h
Outdated
/// \param f: observer callback, that is associated with the given, \p id | ||
/// \param args: arguments associated with the given callback, \p f | ||
template<class F, class ...Args> | ||
void registerObserver(const irep_idt &id, const F &&f, Args&&... args) |
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 don't think we indent here. No CaMelcaSe, please.
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.
done
src/util/code_observer.h
Outdated
} | ||
private: | ||
using observerst = std::function<void(T&)>; | ||
using code_observer_mapt = std::map<const irep_idt, const observerst>; |
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 believe a std::unordered_map
would be preferable.
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.
agreed done
src/util/code_observer.h
Outdated
code_observer_mapt code_observers_map; | ||
}; | ||
|
||
/// \brief codet observer, uses expression visitor to visit all the codet's |
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 comment provides an implementation detail rather than focusing on the big picture. Write documentation by working backwards from the potential user, not for those editing the code.
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.
done
src/util/code_observer.h
Outdated
public code_observer_baset<const codet> | ||
{ | ||
public: | ||
virtual ~const_code_observert() {} |
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.
Unnecessary as implicit due to inheritance from code_observer_baset
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.
done
src/util/code_observer.h
Outdated
{ | ||
forall_symbols(symbol, symbol_table.symbols) | ||
{ | ||
symbolt &w_symbol=symbol_table.get_writeable_ref(symbol->second.name); |
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 a lookup while already iterating over the map! This code should be:
Forall_symbols(it, symbol_table.symbols)
{
it->second.value.visit(*this);
}
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.
"Forall_symbols" does not exist anymore.
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.
Sorry:
for(auto &sym_entry : symbol_table.symbols)
{
sym_entry.second.value.visit(*this);
}
(and someone better clean up symbol_table_baset
, but that's not for this PR)
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.
@tautschnig symbol_table.symbols is constant, so I would need to cast it, which seems ugly to me.
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.
You might want to raise this with @NathanJPhillips or @smowton. I don't recall making or soliciting any of those changes.
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 PR doesn't actually introduce a very essential feature, does it? I'm finding it hard to see the urgency for a half-ready piece of work. The usual approach to visiting various expressions was:
void do_something_rec(const exprt &expr)
{
if(expr.id() == ID_code && to_code(expr).get_statement() == ID_foo)
found_it();
forall_operands(it, expr)
do_something_rec(*it);
}
void do_something(const symbol_tablet &symbol_table)
{
for(const auto &sym_entry : symbol_table.symbols)
do_something_rec(sym_entry.second.value);
}
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.
The PR introduces a utility that help you avoid such boilerplate every time. That's arguably something desirable (but I agree that it's not absolutely essential (but for the same reason constructs such as functions or classes are not essential either)).
The urgency doesn't stem from the PR itself but from our time constraints in the team ;)
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 fully understand that there are time constraints. But putting in a half-baked feature that thus only half-solves problems is a very questionable sort of efficiency. Yes, the boilerplate isn't great. But it's not half-baked, it is simple, and not impacted by people fiddling with APIs. And it's not the first of it's kind. All of this boilerplate code ought to be replaced by a decent visitor. All of them, not half of them. I really don't want to see another depth_iteratort
with its current 2 users.
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 could do a symbol_tablet
iterator quickly that had the semantics of a const_iterator
and so doesn't break sharing/journalling but also included a get_writeable
method that could be called when an element was encountered that you wanted to write to. Is that what you want? You could build a visitor on top of that, though it would be an unusual one as it would need to implement two callbacks, one that checked whether it wanted to write to the symbol and another that got a writable symbol.
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.
@NathanJPhillips we could do that, and then in the non-const version of the code observer/visitor, when the callback is issued along with a reference to a const codet you will get a reference to the symbol_tabelt iterator which will allows you to call get_writeable ref. What do you think? This will avoid the extra look-up but you would still need to walk-down the tree
src/util/code_observer.h
Outdated
{ | ||
const symbolt &w_symbol=symbol_table.lookup_ref(symbol->second.name); | ||
w_symbol.value.visit(*this); | ||
} |
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.
As above, except now as the const version:
forall_symbols(it, symbol_table.symbols)
{
it->second.value.visit(*this);
}
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.
done
src/util/code_observer.h
Outdated
if(expr.id() != ID_code) | ||
return; | ||
notify(to_code(expr)); | ||
}; |
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.
Copy&paste all fixes from above.
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.
done
src/util/code_observer.h
Outdated
}; | ||
}; | ||
|
||
#endif // CPROVER_UTIL_CODE_VISITOR_H |
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.
Please update the name.
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.
done
634e68e
to
c581758
Compare
@tautschnig , should address all of the issues you raised (including tests) |
src/util/code_observer.h
Outdated
public code_observer_baset<codet> | ||
{ | ||
public: | ||
/// iterates through a given symbol table and for every symbol calls it's |
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.
nit pick: s/it's/its/
src/util/code_observer.h
Outdated
{ | ||
symbolt &w_symbol = symbol_table.get_writeable_ref(symbol->second.name); | ||
w_symbol.value.visit(*this); | ||
} |
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.
See comment a few minutes ago to git rid of the duplicate lookup.
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.
@tautschnig, the "Forall_symbols" macro has been removed due to journalling-symbol-ttable
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.
See #1789 (comment):
for(auto &sym_entry : symbol_table.symbols)
{
sym_entry.second.value.visit(*this);
}
unit/util/code_observer.cpp
Outdated
code = codet(ID_skip); | ||
replaced += 1; | ||
} | ||
int replaced; |
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.
You might want a constructor to initialise this to 0?
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 did this one purpose, to make it clear what the value of replaced is in the test, but will change if you prefer
unit/util/code_observer.cpp
Outdated
REQUIRE(code.get_statement() == ID_decl); | ||
|
||
code = codet(ID_skip); | ||
replaced += 1; |
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.
Why not ++replaced;
?
unit/util/code_observer.cpp
Outdated
{ | ||
REQUIRE(code.id() == ID_code); | ||
REQUIRE(code.get_statement() == ID_skip); | ||
count += 1; |
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.
++count;
unit/util/code_observer.cpp
Outdated
add_codet(codet(ID_assign), "ASSIGN1", symbol_table); | ||
|
||
decl_replacert obj; | ||
obj.replaced = 0; |
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.
RAII.
unit/util/code_observer.cpp
Outdated
const codet& code = to_code(it->second.value); | ||
REQUIRE(code.get_statement() != ID_decl); | ||
if(code.get_statement() == ID_skip) | ||
num_skips += 1; |
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.
++
c5e70cb
to
0a2fb4f
Compare
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 agree that the symbol table should be fixed to complete the other half of this implementation (@Degiorgio, please raise an issue for doing this), but I wouldn't make this a condition for getting this PR in. @Degiorgio is saying that the upcoming PRs heavily use this facility and using recursion boilerplate for that would clutter the code. It seems much less work to complete the implementation of the observer (which is a considerable amount of work) in a second PR and effortlessly switch over to the upgraded observer implementation, than redoing everything with custom recursions and changing them back to observers again at a later point in time.
src/util/code_observer.h
Outdated
/// | ||
/// const_code_observert code_observer; | ||
/// code_observer.register_observer(ID_function_call, | ||
/// &A::exmaine_function_calls, |
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.
examine
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.
done
src/util/code_observer.h
Outdated
/// \endcode | ||
/// | ||
/// Note': each unique irep_idt can only be associated with one observer. | ||
|
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.
Please add a note why this doesn't support modifying symbol table entries and what the impact of using this observer for modifying symbol table entries is.
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.
done
Up to you, feel free to override my review. I'd just ask not to create another non-trivial yet hardly used infrastructure -- |
I'd be the last one advocating against uniformity. I see this PR as a step towards replacing these custom iterations by proper patterns. I would be more worried if we introduced parallel infrastructure that does the same thing, and none of them does it well in the end. If there is path towards completing this implementation and eventually making it the standard way of doing things then I'm fine with it. If we think that this will never be the standard way of doing things then we shouldn't introduce this facility. |
Suggested plan of action:
|
0a2fb4f
to
9380275
Compare
@peterschrammel done |
Management over to others - I've loudly expressed all concerns, and the current version is free from problems.
This would be my suggestion for a |
@NathanJPhillips Thanks a lot for the quick turnaround! |
I've provided the two PR's above to create the infrastructure you need. You should now be able to iterate over the symbol table then depth-first iterate over discovered expressions all using constant references. When you finally find something you want to change you can then call |
@NathanJPhillips , great, thanks! will modify the code_observer to make use of the new api's. |
Is it worth re-adding the non-const version now? |
@tautschnig yes, will add the non-const version with new API's Nathan did. |
Hello. I am going forward with closing this PR as it appears stale and no longer congruent with the current state of the repository. If you would like to see this PR through in any case, feel free to reopen the PR and rebase on top of branch |
Visitor for codet, In brief, the class allows you to associate code ID type (example: ID_function_call) with a callback. When executed (by calling visit_symbols) the visitor leverages expr_visitort to explore the given symbol table and issue a callback for all codet's that it knows about.
Example of use:
In this example, all visitor will execute
java_bytecode_languaget::convert_threadblock
for everyID_function_call
in the symbol_table and pass along the appropriate parameters.Note: subscribe can be called multiple times (with different IDs)
Note': the commit, does not include an example of how this is used (this will be coming along in a different pr which will add support for java concurrency)