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.

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

Don't handle model entity names via preprocessor #2226

Open
dweindl opened this issue Dec 4, 2023 · 1 comment
Open

Don't handle model entity names via preprocessor #2226

dweindl opened this issue Dec 4, 2023 · 1 comment
Labels

Comments

@dweindl
Copy link
Member

dweindl commented Dec 4, 2023

The preprocessor defines we are currently using to have more readable model equations are potentially problematic because they aren't scoped. This can lead to issues like:

In file included from /home/runner/work/AMICI/AMICI/test_bmc/Smith_BMCSystBiol2013/stau.cpp:8:
/home/runner/work/AMICI/AMICI/test_bmc/Smith_BMCSystBiol2013/x.h:1: error: "NULL" redefined [-Werror]
    1 | #define NULL x[0]
      | 
In file included from /usr/include/stdio.h:33,
                 from /usr/include/c++/11/cstdio:42,
                 from /usr/include/c++/11/ext/string_conversions.h:43,
                 from /usr/include/c++/11/bits/basic_string.h:6608,
                 from /usr/include/c++/11/string:55,
                 from /usr/include/c++/11/bits/locale_classes.h:40,
                 from /usr/include/c++/11/bits/ios_base.h:41,
                 from /usr/include/c++/11/ios:42,
                 from /home/runner/.local/lib/python3.9/site-packages/amici/include/gsl/gsl-lite.hpp:26,
                 from /home/runner/work/AMICI/AMICI/test_bmc/Smith_BMCSystBiol2013/stau.cpp:5:
/usr/lib/gcc/x86_64-linux-gnu/11/include/stddef.h:392: note: this is the location of the previous definition
  392 | #define NULL __null
      | 

Would be good to come up with some other solution that allows compile-time mapping for readable names to array locations.

Related to #1544

@dweindl dweindl added the c++ label Dec 4, 2023
@dweindl
Copy link
Member Author

dweindl commented Dec 14, 2023

One alternative could be generating, for example, a class Parameters that wraps the plain parameter array and has as members the names of all parameters that map to the respective array location. So what is currently written as p1 + p2 would become p.p1 + p.p2. The main advantage would be, that we have everything inside the class namespace, so it's safe if somebody wants to use a parameter named p. Slightly more verbose, but on the positive side, it would be obvious that those are parameters. It still wouldn't work for c++ reserved names, so no parameter named int etc. would be supported. If we want that, then I guess there is no way around using a string-indexed map.
Seems to be possible to resolve some string-based mapping at compile time (e.g. https://github.com/serge-sans-paille/frozen/). However, that would make the code less readable, e.g. p["p1"] + p["p2"] (or probably worse, p[_p["p1"]] + p[_p["p2"]], because we need to look up indices, not values, as the parameter values aren't known at compile time).

EDIT: Okay, a simple consteval function for resolving string to index mapping would work without additional run-time cost, but it would require writing something like p[_p("p1")] + p[_p("p2")], or with a bit of macros (and code printing adaptations) PAR("p1") + PAR("p2"), which is actually not that bad. At least we wouldn't have to bother with any reserved names in amici/c++/... . Happy about better solutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant