Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor chores: TURB_MODEL now enum class. #1404

Merged
merged 12 commits into from
Oct 19, 2021
Merged

Minor chores: TURB_MODEL now enum class. #1404

merged 12 commits into from
Oct 19, 2021

Conversation

TobiKattmann
Copy link
Contributor

@TobiKattmann TobiKattmann commented Oct 17, 2021

Hi all,

I made the ENUM_TURB_MODEL an enum class. I will add a few other small things in here. In case you have some minor cleanup commit feel free to push in here

Also made some turbSA+SST functions final where it was override previously

Related Work

#1220 #1242 and probably some others on the road to enum class

@suargi i hope this will not interfere to heavily with your work on the SA model things

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.

Went well for the most part. In some switches a default: break; had to
be added. in option_structure.inl the old ENUM_TURB_MODEL was stored in
some unsigned short intInfo container which is not seamlessly possible
as an enum class names its own new type. Imo the storage is not not
necessary as it is never used so I just removed it.
Common/src/CConfig.cpp Outdated Show resolved Hide resolved
Comment on lines -1907 to -1908
this->intInfo[i][0] = iit->second;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in the commit message: From my grep-ping I believe this can be safely removed. The intInfo is used for some other WallFunctions to count some amount of nodes which is used in other instances. In this particular instances it stored the enum ENUM_TURB_MODEL which is of course an unsigned short. With enum class that is no longer possible. Therefore I removed it because I think this info is never used

@TobiKattmann
Copy link
Contributor Author

TobiKattmann commented Oct 17, 2021

CodeFactor has

  • 1 Complex code ...
  • 1 tab use in output_structure_legacy.cpp (fixed ✔️ )

@TobiKattmann
Copy link
Contributor Author

Now CodeFactor has Complex Code twice and nothing else

Copy link
Member

@pcarruscag pcarruscag left a comment

Choose a reason for hiding this comment

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

Revert 8f3087b so we can keep some consistency in the code.
"override" is the default for anything virtual, "final" is for special purposes.

@TobiKattmann
Copy link
Contributor Author

Thanks for the reviewing and helpful comments @pcarruscag 👍 I'll merge this once the hybrid_AD passes by chance .. i know that's not strictly necessary but feels better

@TobiKattmann TobiKattmann merged commit d8cfe78 into develop Oct 19, 2021
@TobiKattmann TobiKattmann deleted the minor_chores branch October 19, 2021 15:02
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