Simplify if statement with container #3047
Conversation
JCGoran
force-pushed
the
jelic/simplify_if
branch
from
266eacd
to
a039b04
Compare
|
✔️ a039b04 -> Azure artifacts URL |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3047 +/- ##
==========================================
- Coverage 67.28% 67.26% -0.02%
==========================================
Files 571 571
Lines 104913 104877 -36
==========================================
- Hits 70588 70546 -42
- Misses 34325 34331 +6 ☔ View full report in Codecov by Sentry. |
This comment has been minimized.
This comment has been minimized.
src/nmodl/solve.cpp
Outdated
| method = (Symbol*) 0; | ||
| lq->element.sym = (Symbol*) 0; | ||
| cvodemethod_ = 3; | ||
| const std::unordered_map<std::string, int> methods = {{"after_cvode", 1}, |
There was a problem hiding this comment.
With such a very low number of items maybe underered_map is bit overkill?
There was a problem hiding this comment.
Done, though I will admit that now the code is IMHO less readable since the unordered_map API is much nicer than messing with pair in a vector.
There was a problem hiding this comment.
std::map is much more readable! See comment below.
There was a problem hiding this comment.
Perhaps it's an aside, but in my experience using containers will increase the code significantly compared to current string compairisons. On every function invocation, the container will likely be rebuilt.
For instance, in this godbolt, I copied the vector impl: https://godbolt.org/z/1naM8oW1o
Compared to a string compare, it's nearly 6 times longer, ASM wise.
There was a problem hiding this comment.
I guess my question in that case is: is this part of the code performance-critical? If so, I'm fine with leaving the current implementation (still would like to fix that typo though).
As a(nother) side note: the unordered_map version is only ~2x longer than the current one (only w/GCC, w/clang the difference is more like 5-6x, though still shorter than vector or map): https://godbolt.org/z/4a8Peo3Wb
There was a problem hiding this comment.
Maybe make it static const to avoid reconstructing the map? I don't think that ASM size really matters here - but I would also move the map very close to the enum definition to lessen the chance of divergence.
- fix a bug: `cvode_v` should have been `cvode_t_v` - use an internal `enum class` to avoid the above happening again
JCGoran
changed the title
unordered_map
|
✔️ 5f91500 -> Azure artifacts URL |
This comment has been minimized.
This comment has been minimized.
src/nmodl/solve.cpp
Outdated
| const std::vector<std::pair<std::string, CVodeMethod>> cvode_methods = { | ||
| {"after_cvode", CVodeMethod::after_cvode}, | ||
| {"cvode_t", CVodeMethod::cvode_t}, | ||
| {"cvode_t_v", CVodeMethod::cvode_t_v}}; | ||
|
|
||
| const auto& method_match = | ||
| std::find_if(cvode_methods.begin(), cvode_methods.end(), [&method](const auto& item) { | ||
| return method && method->name == item.first; | ||
| }); | ||
| if (method && method_match != cvode_methods.end()) { | ||
| cvodemethod_ = method_match->second; |
There was a problem hiding this comment.
So... this is basically like a map lookup?
| const std::vector<std::pair<std::string, CVodeMethod>> cvode_methods = { | |
| {"after_cvode", CVodeMethod::after_cvode}, | |
| {"cvode_t", CVodeMethod::cvode_t}, | |
| {"cvode_t_v", CVodeMethod::cvode_t_v}}; | |
| const auto& method_match = | |
| std::find_if(cvode_methods.begin(), cvode_methods.end(), [&method](const auto& item) { | |
| return method && method->name == item.first; | |
| }); | |
| if (method && method_match != cvode_methods.end()) { | |
| cvodemethod_ = method_match->second; | |
| const std::map<std::string, CVodeMethod> cvode_methods = { | |
| {"after_cvode", CVodeMethod::after_cvode}, | |
| {"cvode_t", CVodeMethod::cvode_t}, | |
| {"cvode_t_v", CVodeMethod::cvode_t_v}}; | |
| decltype(cvode_methods)::const_iterator m; | |
| if (method && (m = cvode_methods.find(method)) != cvode_methods.end()) { | |
| cvodemethod_ = m->second; |
|
|
✔️ 53fb177 -> Azure artifacts URL |
| cvodemethod_ = CVodeMethod::nomethod; | ||
| if (method && cvode_method_map.count(method->name)) { | ||
| cvodemethod_ = cvode_method_map.at(method->name); |
There was a problem hiding this comment.
| cvodemethod_ = CVodeMethod::nomethod; | |
| if (method && cvode_method_map.count(method->name)) { | |
| cvodemethod_ = cvode_method_map.at(method->name); | |
| cvodemethod_ = deduce_cvode_method(method); | |
| if (cvodemethod_ != CVodeMethod::none) { |
You can implement deduce_cvode_method in two stages, first convert method to a char*. Then convert the char* to enum, returning CVodeMethod::none if the string isn't one of the supported names for the cvode methods.
Trivial simplification in
solve.cpp. Note that I'm fairly sure this also fixes a subtle bug: thestrcmpwas comparingcvode_v, which should've actually been calledcvode_t_v.