Refactor removing module elements#2489
Conversation
This creates utility functions for removing module elements: removing one element by name, and removing multiple elements using a predicate function. And makes other parts of code use it. I think this is a light-handed approach than calling `Module::updateMaps` after removing only a part of module elements. This also fixes a bug in the inlining pass: it didn't call `Module::updateMaps` after removing functions. After this patch callers don't need to additionally call it anyway.
tlively
left a comment
tlively
left a comment
There was a problem hiding this comment.
Nice! Just a couple questions.
| if (functionTypes[i]->name == name) { | ||
| functionTypes.erase(functionTypes.begin() + i); | ||
| template<typename Vector, typename Map> | ||
| void removeModuleElement(Vector& v, Map& m, Name name) { |
There was a problem hiding this comment.
Would it make sense to implement this in terms of removeModuleElements?
There was a problem hiding this comment.
That might be slower, I worry...
There was a problem hiding this comment.
For the vector thing the effect would be more or less the same, for the deletion from the map, this can be done in constant time whereas we iterate through all elements in the map in removeModuleElements. Given that a lot of usage in the codebase is removing a single element, I guess it'd be better to keep this in the way it is?
| std::function<bool(Elem* elem)> pred) { | ||
| v.erase( | ||
| std::remove_if(v.begin(), v.end(), [&](auto& e) { return pred(e.get()); }), | ||
| v.end()); |
There was a problem hiding this comment.
Why do a remove with iterators and then also do the remove with a loop?
There was a problem hiding this comment.
The first one is deleting from a vector and the second is from a map. Actually I think I should swap the order of these two, because deleting unique_ptr destroys the object so the second map thing can segfault.
kripken
left a comment
kripken
left a comment
There was a problem hiding this comment.
Very nice idea and implementation!
This does something similar to WebAssembly#2489 for more functions, removing boilerplate code for each module element using template functions.
This does something similar to WebAssembly#2489 for more functions, removing boilerplate code for each module element using template functions.
This does something similar to WebAssembly#2489 for more functions, removing boilerplate code for each module element using template functions.
This does something similar to #2489 for more functions, removing boilerplate code for each module element using template functions.
3 participants
This creates utility functions for removing module elements: removing
one element by name, and removing multiple elements using a predicate
function. And makes other parts of code use it. I think this is a
light-handed approach than calling
Module::updateMapsafter removingonly a part of module elements.
This also fixes a bug in the inlining pass: it didn't call
Module::updateMapsafter removing functions. After this patch callersdon't need to additionally call it anyway.