Skip to content

Part 2: Make enum -> enum class - #1258

Merged
TobiKattmann merged 19 commits into
developfrom
remove_enum_numbers
Apr 21, 2021
Merged

Part 2: Make enum -> enum class#1258
TobiKattmann merged 19 commits into
developfrom
remove_enum_numbers

Conversation

@TobiKattmann

@TobiKattmann TobiKattmann commented Apr 7, 2021

Copy link
Copy Markdown
Contributor

Hi all,

this is the continuation of #1242 . Change of the enum's in option_structure.hpp into enum class. This has the benefit of no implicit conversion to int and therefore increased safety with the enum type -> c++ core guidelines.

If you want to sponsor a commit (which I highly recommend ;) ) then a good approach is to

  1. make your enum of choice an enum class and remove the associated numbers.
  2. Rename here if reasonable, i,.e. ENUM_INLET_TYPE can be just INLET_TYPE. Also the option names to not be redundant so e.g. INLET_TYPE::PRESSURE_INLET_TYPE could become INLET_TYPE::PRESSURE
  3. Add a NONE or NO_THING option if needed. Often the global NONE option is and that made a lot of people very angry and has been widely regarded as a bad move 🧙
  4. then you go to CConfig.c/hpp and change the option type and everything there (because in the CConfig class the variable is usually defined). Find the GetKind_yourOption and search the code base for that Get function.
  5. Once all search results and the following function bodies are changed just compile and fix until the code builds
  6. Special care for switch statements: If not all options are present make a default; break; line -> otherwise the automated builds will fail on a warning (clang is used, I found gcc does only complain with an additional flag)
  7. commit at best just 1 commit with a clear message and push. That makes reverting easier if it fails and introduced some non-obvious error

Have fun :)

Related Work

#1242

PR Checklist

Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.

  • I am submitting my contribution to the develop branch.
  • My contribution generates no new compiler warnings (try with the '-Wall -Wextra -Wno-unused-parameter -Wno-empty-body' compiler flags, or simply --warnlevel=2 when using meson).
  • My contribution is commented and consistent with SU2 style.
  • I have added a test case that demonstrates my contribution, if necessary.
  • I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp) , if necessary.

@pr-triage pr-triage Bot added the PR: draft label Apr 7, 2021
Comment thread Common/include/option_structure.hpp Outdated
MESH_DEFORM = 3,
SOLUTION_AND_MESH = 4
enum class RECORDING {
NO_RECORDING,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added a NO_RECORDING option as the global NONE was used

Comment thread SU2_CFD/src/drivers/CDiscAdjMultizoneDriver.cpp Outdated
@TobiKattmann TobiKattmann mentioned this pull request Apr 7, 2021
3 tasks
/*!
* \brief Types of mathematical problem to solve
*/
enum ENUM_MATH_PROBLEM {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

not used any more, nice 👍

@TobiKattmann
TobiKattmann marked this pull request as ready for review April 21, 2021 13:18
@pr-triage pr-triage Bot removed the PR: draft label Apr 21, 2021
@TobiKattmann TobiKattmann changed the title [WIP] Part 2: Make enum -> enum class Part 2: Make enum -> enum class Apr 21, 2021
@TobiKattmann

Copy link
Copy Markdown
Contributor Author

Codefactor is just complainig about the usual CIncEulerSolver. All other test pass, so I'll merge this and continue in Part III.
Coming to a repository near you soon. 🎦

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants