Skip to content

Add BOUT_ENUM_CLASS_NS and switch bout::ConductionMethod to it - #3457

Merged
dschwoerer merged 7 commits into
nextfrom
BOUT_ENUM_CLASS_NS
Aug 14, 2026
Merged

Add BOUT_ENUM_CLASS_NS and switch bout::ConductionMethod to it#3457
dschwoerer merged 7 commits into
nextfrom
BOUT_ENUM_CLASS_NS

Conversation

@dschwoerer

Copy link
Copy Markdown
Contributor

BOUT_ENUM_CLASS_NS is a version of BOUT_ENUM_CLASS that can create namespaced enums

A version of BOUT_ENUM_CLASS that can create namespaced enums

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

Comment thread include/bout/bout_enum_class.hxx
Comment thread include/bout/difops.hxx
@ZedThree

Copy link
Copy Markdown
Member

Might be better/simpler if we just stuck a namespace onto the existing BOUT_ENUM? We can then do either of the following for backwards compatibility:

// bring the Foo name into the current scope
using bout::Foo;
// type alias lets us put a deprecated attribute on it
using Foo [[deprecated("use bout::Foo")]] = bout::Foo;

@ZedThree

Copy link
Copy Markdown
Member

Probably the std::int8_t can also be changed to std::uint8_t, I seem to recall it always triggers clang-tidy warnings

@dschwoerer

Copy link
Copy Markdown
Contributor Author

Might be better/simpler if we just stuck a namespace onto the existing BOUT_ENUM? We can then do either of the following for backwards compatibility:

// bring the Foo name into the current scope
using bout::Foo;
// type alias lets us put a deprecated attribute on it
using Foo [[deprecated("use bout::Foo")]] = bout::Foo;

I think it might make sense to introduce the new namespaced enum first, and test it, before we do a lot of churn?

Probably the std::int8_t can also be changed to std::uint8_t, I seem to recall it always triggers clang-tidy warnings

It should probably be unsigned, but I think clang-tidy is happy with this one. It was not happy with int - which is the default and the reason I switched to an 8bit type in d170774.

@ZedThree

Copy link
Copy Markdown
Member

What needs testing? It's just putting the declaration into a namespace -- if it compiles, the existing tests should be sufficient?

@dschwoerer

dschwoerer commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

If the tests cover everything, than that is probably true.

It is still a lot of churn, and I do not fancy doing it right now, especially if we deprecate the old version (which we should) and switch everywhere away from it (to avoid more warnings).

@bendudson

bendudson commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

For my understanding, what's the difference between this and just using

namespace bout {
  BOUT_ENUM_CLASS(myclass, ...);
}

rather than

BOUT_ENUM_CLASS_NS(bout, myclass, ...)

Edit: Ah I see: specializations of toString, Options::as shouldn't be in the namespace.

Use `toString` to convert an unhandled ConductionMethod into a string.
Should be easier for a user to understand than an integer.
@ZedThree

Copy link
Copy Markdown
Member

I mean, they probably should be in the bout namespace, we're very inconsistent on this point!

@dschwoerer
dschwoerer merged commit b445c7e into next Aug 14, 2026
23 checks passed
@dschwoerer
dschwoerer deleted the BOUT_ENUM_CLASS_NS branch August 14, 2026 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants