Make "enum" : "enum class" thus removing enum numbers #1242

TobiKattmann · 2021-03-23T13:45:02Z

Proposed Changes

Hi all,

The title gives it away. I imagine that piece by piece some commits are chipped in to remove more and more enum numbers. Like that it is more obvious if sth breaks because somewhere someone uses builds on knowing the actual number to do some shenanigans.
Maybe just removing all in a big blow saves time bit #1220 already shows some limitations.

I used the following command (for streamwise periodic in this case) and looked over the results whether I notice sth fishy. Not perfect but might help catchin some easy problems.

grep -r 'NO_STREAMWISE_PERIODIC\|PRESSURE_DROP\|STREAMWISE_MASSFLOW\|Streamwise_Periodic_Map\|GetKind_Streamwise_Periodic' SU2_CFD SU2_DOT SU2_GEO SU2_DEF Common --include=*cpp --include=*hpp --include=*inl

So feel free to commit your removals in here and if it breaks and one doesn't want to search for it just revert that and mention the specific enum so that no-one repeats that

Happy enum-removal

Related Work

@bigfooted mentioned that a couple of times and #1220 discusses some limitations

PR Checklist

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.

pcarruscag · 2021-03-23T15:44:04Z

Provocative thought of the day: Removing the numbers from the enums is a bit of spring cleaning, the "modern C++" way is to use enum class and to never rely on implicit conversions from enum to integer type.
In any case, it should be safe to remove the numbers when they already start at zero.

TobiKattmann · 2021-03-23T17:55:01Z

the "modern C++" way is to use enum class and to never rely on implicit conversions from enum to integer type.

I am in. It is a little (actually quite a bit) more involved as the enum namespace has to be given now everywhere an entry is used.
I was not familiar with enum vs enum class if someone else needs an entry point stack overflow, playin around in compiler explorer... oh and of course the c++ core guidelines entry endorses the use of enum class as well

Common/include/option_structure.inl

Common/src/CConfig.cpp

pcarruscag · 2021-03-23T18:16:34Z

Common/include/option_structure.hpp

+  MakePair("NONE",          ENUM_STREAMWISE_PERIODIC::NONE)
+  MakePair("PRESSURE_DROP", ENUM_STREAMWISE_PERIODIC::PRESSURE_DROP)
+  MakePair("MASSFLOW",      ENUM_STREAMWISE_PERIODIC::MASSFLOW)


It ends up being a little more verbose but you get to use "names" that would otherwise conflict with what is already in use for objective functions and things like that.

…nst ref.

pcarruscag · 2021-03-24T12:42:31Z

Common/include/option_structure.inl

      str.append(": invalid option value ");
      str.append(option_value[0]);
      str.append(". Check current SU2 options in config_template.cfg.");
      return str;
    }
    // If it is there, set the option value
-    Tenum val = this->m[option_value[0]];
-    this->field = val;
+    field = static_cast<TField>(it->second);


The problem you were having is because operator [ ] modified the map if the keys does not exist yet.
But since we use find (which does not modify) we can just store the result and de-reference the iterator instead of "accessing" the map again.

pcarruscag · 2021-03-24T12:44:23Z

Common/include/option_structure.inl

-  void SetDefault() override {
-    this->field = this->def;
-  }
+  void SetDefault() override { field = static_cast<TField>(def); }


I added the static cast but this was not smart 🤦 (it would make enum class compatible with unsigned short...)

pcarruscag · 2021-03-24T12:44:46Z

Common/include/option_structure.inl

+template <class Tenum, class TField>
+class COptionEnumList final : public COptionBase {


available for lists

pcarruscag · 2021-03-24T12:45:42Z

Common/src/CConfig.cpp

-template <class Tenum>
-void CConfig::addEnumOption(const string name, unsigned short & option_field, const map<string, Tenum> & enum_map, Tenum default_value) {
+template <class Tenum, class TField>
+void CConfig::addEnumOption(const string name, TField& option_field, const map<string,Tenum>& enum_map, Tenum default_value) {


The same "add" function can be used without changes, we just let the compiler deduce the template parameters.

pcarruscag · 2021-03-24T12:46:59Z

Common/src/geometry/CGeometry.cpp

-                                        const vector<pair<unsigned short,su2double> > &kernels,
+                                        const vector<pair<ENUM_FILTER_KERNEL,su2double> > &kernels,


enum class is the safe way to remove the numbers... it breaks the code everywhere :)

lgtm-com · 2021-03-24T13:43:24Z

This pull request introduces 2 alerts when merging bd95226 into 949f4e5 - view on LGTM.com

new alerts:

2 for Resource not released in destructor

lgtm-com · 2021-03-26T18:07:17Z

This pull request introduces 2 alerts when merging 5492805 into 14d7724 - view on LGTM.com

new alerts:

2 for Resource not released in destructor

…ment.

TobiKattmann · 2021-03-29T16:56:55Z

SU2_CFD/src/solvers/CAdjEulerSolver.cpp

+
+      default:
+        SU2_MPI::Error("Unsupported INLET_TYPE.", CURRENT_FUNCTION);
+        break;


Now that the compiler gets to handle the enum explicitly, it complains if not all cases are listed.
I compile with gcc 5.3.0 and warnlevel=3 and could compile without any warning. The CI build with clang failed due to that warning.

Adding -Wswitch-enum to gcc shows those places but the warning won't go away by adding a simple default: break; like it does for clang, i.e. it is a bit more restrictive. So when using gcc find those spots with the additional warning -> add the default (and error if you like) -> no need to add all options.

Maybe we could be kinder on that warning as well, but on the other hand it is a bit more explicit this way

TobiKattmann · 2021-04-06T11:25:54Z

SU2_CFD/src/solvers/CIncEulerSolver.cpp

@@ -1597,7 +1597,7 @@ void CIncEulerSolver::Source_Residual(CGeometry *geometry, CSolver **solver_cont

      /*--- Get the physical time. ---*/
      su2double time = 0.0;
-      if (config->GetTime_Marching()) time = config->GetPhysicalTime();
+      if (config->GetTime_Marching() != TIME_MARCHING::STEADY) time = config->GetPhysicalTime();


Watch out for: implicitly relying that STEADY evaluates to zero/false and everything else to >0 and therefore true.

TobiKattmann · 2021-04-06T11:28:53Z

SU2_CFD/src/solvers/CEulerSolver.cpp

@@ -1087,7 +1087,7 @@ void CEulerSolver::SetNondimensionalization(CConfig *config, unsigned short iMes
  su2double Beta          = config->GetAoS()*PI_NUMBER/180.0;
  su2double Mach          = config->GetMach();
  su2double Reynolds      = config->GetReynolds();
-  bool unsteady           = (config->GetTime_Marching() != NO);
+  bool unsteady           = (config->GetTime_Marching() != TIME_MARCHING::STEADY);


using another enum's value like NO in this case. Note to self: Maybe introduce an inline bool isdual_time and isunsteady in option_structure as it is used quite often

lgtm-com · 2021-04-07T15:12:53Z

This pull request introduces 2 alerts when merging e27da1d into 3b20982 - view on LGTM.com

new alerts:

2 for Resource not released in destructor

TobiKattmann · 2021-04-07T15:32:43Z

Merging this now to keep the PR and the number of problems, that will arise in other code, in bounds. This work will be continued in a future PR.

If your code breaks after merging this PR you most likely just have to add an <ENUM_NAME>:: in front of your enum variable.

Thanks @pcarruscag for the idea and the setup in option_structure.inl

Remove Enum number for streamwise periodic flow

5fb5bb2

TobiKattmann changed the title ~~[WIP] Remove Enum number for streamwise periodic flow~~ [WIP] Remove Enum numbers Mar 23, 2021

TobiKattmann added the changelog:chore label Mar 23, 2021

TobiKattmann added 2 commits March 23, 2021 18:46

Add addEnumClassOption to make enum class possible.

63ce3c4

Make ENUM_STREAMWISE_PERIODIC an enum class.

62cad07

pcarruscag reviewed Mar 23, 2021

View reviewed changes

Common/include/option_structure.inl Outdated Show resolved Hide resolved

TobiKattmann commented Mar 23, 2021

View reviewed changes

Common/src/CConfig.cpp Outdated Show resolved Hide resolved

pcarruscag reviewed Mar 23, 2021

View reviewed changes

TobiKattmann and others added 3 commits March 24, 2021 00:46

COptionEnum with 2 template params to avoid duplication. Map now a co…

a5257d9

…nst ref.

Revert const ref map because the tests didn't compile.

f3ac7b1

spring cleaning

8f17be4

pcarruscag reviewed Mar 24, 2021

View reviewed changes

thou shall not cast enum class

bd95226

Merge branch 'develop' into remove_enum_numbers

5492805

TobiKattmann added 3 commits March 29, 2021 17:46

Enum class OUTLET_TYPE -> INC_OUTLET_TYPE

75d172c

Make INLET_TYPE enum class.

ad62f4c

Resolve enum class compile error: enum value not handled switch state…

094b75f

…ment.

TobiKattmann commented Mar 29, 2021

View reviewed changes

TobiKattmann added 8 commits March 30, 2021 20:28

Make SU2_COMPONENT enum class.

bdf694d

Enum class SU2_COMPONENT for UnitTests as well.

e7d3d95

Enum class SU2_COMPONENT for SU2_PY... i hope at least.

cda58b7

Merge remote-tracking branch 'origin/develop' into remove_enum_numbers

c4ea172

Made ENUM_REGIME an enum class

bb9c018

Make INC_DENSITYMODEL an enum class.

f09ac34

Make CHT_COUPLING an enum class.

d93ab4f

Make TIME_MARCHING an enum class.

bfb3c9f

TobiKattmann commented Apr 6, 2021

View reviewed changes

Merge branch 'develop' into remove_enum_numbers

61834c9

su2code deleted a comment from lgtm-com bot Apr 7, 2021

minor build fix after develop merge

e27da1d

pcarruscag approved these changes Apr 7, 2021

View reviewed changes

TobiKattmann changed the title ~~[WIP] Remove Enum numbers~~ [WIP] Make "enum" : "enum class" thus removing enum numbers Apr 7, 2021

TobiKattmann changed the title ~~[WIP] Make "enum" : "enum class" thus removing enum numbers~~ Make "enum" : "enum class" thus removing enum numbers Apr 7, 2021

pr-triage bot added the PR: reviewed-approved label Apr 7, 2021

TobiKattmann merged commit 1f7f6bb into develop Apr 7, 2021

TobiKattmann deleted the remove_enum_numbers branch April 7, 2021 15:35

pr-triage bot added PR: merged and removed PR: reviewed-approved labels Apr 7, 2021

This was referenced Apr 7, 2021

Part 2: Make enum -> enum class #1258

Merged

Fix restart file writing for time convergence and 2nd order time stepping #1237

Merged

TobiKattmann mentioned this pull request Aug 5, 2021

NEMO - Improvements on the Preprocessing phase and inclusion of Chapmann-Enskog for Mutation++ #1343

Merged

TobiKattmann mentioned this pull request Oct 17, 2021

Minor chores: TURB_MODEL now enum class. #1404

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make "enum" : "enum class" thus removing enum numbers #1242

Make "enum" : "enum class" thus removing enum numbers #1242

TobiKattmann commented Mar 23, 2021

pcarruscag commented Mar 23, 2021

TobiKattmann commented Mar 23, 2021

pcarruscag Mar 23, 2021

pcarruscag Mar 24, 2021

pcarruscag Mar 24, 2021

pcarruscag Mar 24, 2021

pcarruscag Mar 24, 2021

pcarruscag Mar 24, 2021

pcarruscag Mar 24, 2021

lgtm-com bot commented Mar 24, 2021

lgtm-com bot commented Mar 26, 2021

TobiKattmann Mar 29, 2021

TobiKattmann Apr 6, 2021

TobiKattmann Apr 6, 2021

lgtm-com bot commented Apr 7, 2021

TobiKattmann commented Apr 7, 2021

		template <class Tenum, class TField>
		class COptionEnumList final : public COptionBase {

		const vector<pair<unsigned short,su2double> > &kernels,
		const vector<pair<ENUM_FILTER_KERNEL,su2double> > &kernels,

Make "enum" : "enum class" thus removing enum numbers #1242

Make "enum" : "enum class" thus removing enum numbers #1242

Conversation

TobiKattmann commented Mar 23, 2021

Proposed Changes

Related Work

PR Checklist

pcarruscag commented Mar 23, 2021

TobiKattmann commented Mar 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Mar 24, 2021

lgtm-com bot commented Mar 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Apr 7, 2021

TobiKattmann commented Apr 7, 2021