Skip to content
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.

Sign up for GitHub

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

Simplify if statement with container #3047

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Simplify if statement with container #3047

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a039b04
Simplify if statement with `unordered_map`
JCGoran Aug 14, 2024
ab86a18
Use `vector` instead of `unordered_map`
JCGoran Aug 15, 2024
5f91500
Include `algorithm` header for `std::find_if`
JCGoran Aug 15, 2024
53fb177
Use a global `unordered_map`
JCGoran Aug 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 9 additions & 14 deletions src/nmodl/solve.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@
#include "symbol.h"

#include <cstdlib>
#include <string>
#include <unordered_map>

/* make it an error if 2 solve statements are called on a single call to
model() */
Expand Down Expand Up @@ -126,20 +128,13 @@
lq = lq->next;
method = SYM(lq);
cvodemethod_ = 0;
if (method && strcmp(method->name, "after_cvode") == 0) {
method = (Symbol*) 0;
lq->element.sym = (Symbol*) 0;
cvodemethod_ = 1;
}
if (method && strcmp(method->name, "cvode_t") == 0) {
method = (Symbol*) 0;
lq->element.sym = (Symbol*) 0;
cvodemethod_ = 2;
}
if (method && strcmp(method->name, "cvode_v") == 0) {
method = (Symbol*) 0;
lq->element.sym = (Symbol*) 0;
cvodemethod_ = 3;
const std::unordered_map<std::string, int> methods = {{"after_cvode", 1},
Copy link
Member

@ferdonline ferdonline Aug 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With such a very low number of items maybe underered_map is bit overkill?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::map is much more readable! See comment below.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{"cvode_t", 2},

Check warning on line 132 in src/nmodl/solve.cpp

View check run for this annotation

Codecov / codecov/patch

src/nmodl/solve.cpp#L131-L132 

Added lines #L131 - L132 were not covered by tests
{"cvode_v", 3}};
if (method && methods.count(method->name)) {
cvodemethod_ = methods.at(method->name);
method = nullptr;
lq->element.sym = nullptr;
}
lq = lq->next;
errstmt = LST(lq);
Expand Down
Loading