I’m going to try to simplify code steadily and slowly. If anyone wants to help, happy to get it.
1 Like
I’m starting with this PR: Expanding macros directly into code. by syclik · Pull Request #2965 · stan-dev/math · GitHub
I’m trying to remove the mixed use of macros + template metaprograms. It’s pretty hard to maintain code when you can’t find the code and have to expand the macros to find it. It’s also hard to tell that it’s actually a macro expansion due to the naming.
So I’m first expanding it. Then I’ll simplify.
Is the plan what I heard at the Stan meeting, which is to unfold all the macros then throw away instantiations that aren’t used? That feels like it’s going to bloat the code base and remove potential things we’ll need going forward. That is, someone may try to look something up and it just won’t exist now because it hadn’t been used until now.
Is there a way that this could be done with template so that it’s complete but not huge?
Hi @Bob_Carpenter, thanks for the comments!
Just FYI, my first concern before trying it out was exactly what you mentioned, we’d “remove potential things we’ll need going forward.” After putting in the work to do it and then literally walking through every single remaining requires_* that is in use, I think it’s much less of a concern. (Most of the time I’d lean towards completeness in an API for consistency, but here we’re just not using most of the framework that’s built.)
There’s an open PR that implements the changes and you can take a look for yourself: Expanding 5 macros `STAN_ADD_REQUIRE_*` directly into code. by syclik · Pull Request #2965 · stan-dev/math · GitHub. It’s a net +1500 lines, so there is some bloat added. (Roughly half of the new lines are doxygen comments.)
I have a long list in my head of why this change outweighs having the macros. I’ll spare us a long discourse post (unless you’d like more support). I think the most important reasons are cognitive load and future maintenance. With the macros expanded, the requires_* template metaprograms are clear and not complicated. (It’s really hard to find the definition through the macro; if you were to ask a C++ person that didn’t know Math library in depth, I think it’d be very difficult for that person to know what any given requires_* template metaprogram does.) On the maintenance side, the set of macros aren’t really testable, isn’t easy to document, and really hard to change without knowing downstream effects of any change (minor or major). Btw, it’s really hard to come up with a system to test and document macros.
After expanding the macros, I’ve started seeing some incorrect implementations of the template metaprograms as they were expanded. They should be addressed, but using the macros makes it very difficult. Any change to a macro will propagate everywhere and some of the incorrect uses aren’t applicable everywhere.
The major downside to this change is the flipside of the last paragraph: having the macros allows us to make a sweeping change all at once. There are ~30 files that use the macros right now. That said, I still lean towards the change because there are a lot more benefits than drawbacks.