sxs-collaboration / spectre

SpECTRE is a code for multi-scale, multi-physics problems in astrophysics and gravitational physics.
https://spectre-code.org
Other
159 stars 189 forks source link

Add TaggedVariant, parsable similarly to a factory #6096

Closed wthrowe closed 2 months ago

wthrowe commented 3 months ago

I have mixed feelings on the visit interface. The version here follows std::visit and allows multiple TaggedVariants to be passed to a single call, but this makes reporting the tag awkward and, in particular, prevents implicit conversions between reference types in the visitor arguments. An alternative would be to restrict to the (I assume vastly most common) case of a single TaggedVariant and pass the tag and value as separate arguments, which would fix all the conversion trouble. Opinions are welcome.

Ping @knelli2

Proposed changes

Upgrade instructions

Code review checklist

Further comments

nilsdeppe commented 3 months ago

For what it's worth, I added multi-container gets https://github.com/sxs-collaboration/spectre/blob/develop/src/DataStructures/TaggedContainers.hpp at some point because, while not the most common case, we did need it

wthrowe commented 3 months ago

Apparently the cppreference compiler support tables are wrong or incomplete, so I'll have to figure out what compiler versions constexpr support works in. Clang 17 and gcc 13 are known working. gcc 9 was expected to fail, so it does runtime tests instead of static assertions, so I'll switch other versions to that once I figure out what they are.

wthrowe commented 3 months ago

The first working gcc version is 12. I can't easily test clang because clang 15 can't compile DomainCreators and my distro's clang 16 package won't install for some reason I don't feel like debugging. I'm guessing the changes happened at the same time as constexpr std::vector, which was gcc 12 and clang 15, so that's what I went with there.