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

Restructure CVariable files #725

Merged
merged 6 commits into from
Jul 8, 2019
Merged

Conversation

pcarruscag
Copy link
Member

Proposed Changes

Breakup CVariable classes into individual hpp and cpp files, with inlines defined in hpp.
Revise the includes of client classes (CSolver and derived).

Related Work

Addresses part of #583
Preparation for #716

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).
  • My contribution is commented and consistent with SU2 style.
  • I have added a test case that demonstrates my contribution, if necessary.

../../Common/src/toolboxes/MMS/libSU2_a-CUserDefinedSolution.o
___bin_SU2_DEF_LDADD = \
../../SU2_CFD/obj/libSU2Core.a \
../../Common/lib/libSU2.a
Copy link
Member Author

Choose a reason for hiding this comment

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

Is there any reason to link against individual object files instead of the static library?

Copy link
Member

Choose a reason for hiding this comment

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

Actually there is no reason 👍 Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

agreed.. I think that's just legacy behavior from before the libSU2Core.a was added around the time of the in-memory python wrapper

../../Common/src/toolboxes/MMS/libSU2_a-CUserDefinedSolution.o
___bin_SU2_DEF_LDADD = \
../../SU2_CFD/obj/libSU2Core.a \
../../Common/lib/libSU2.a
Copy link
Member

Choose a reason for hiding this comment

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

agreed.. I think that's just legacy behavior from before the libSU2Core.a was added around the time of the in-memory python wrapper

@@ -0,0 +1,150 @@
/*!
* \file CAdjEulerVariable.hpp
Copy link
Member

Choose a reason for hiding this comment

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

Looks like you are already doing it, but this is a nice opportunity to fix any indentation / white space / line ending issues when creating these new files. Can you please double check?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think indentation was already ok, I striped all trailing white spaces now.
There are also many (many...) comments that were copied and do not match the function, that is not a huge deal since these methods explain themselves. I fixed some but I will not fix all of them...

Copy link
Member

Choose a reason for hiding this comment

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

Certainly not expected that you fix them all.. let's get some help on that in a later sweep once merged.

Thanks for the clean up! Can you also verify that all line endings are linux style and not windows format?

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently everything was LF-ended already, at least the IDE I use did not pick up anything.

Copy link
Member

@talbring talbring left a comment

Choose a reason for hiding this comment

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

Thanks @pcarruscag! Just went through all the changes and I have nothing to complain. Lets continue with this restructuring ... I think it will really help with the readability and maintenance.

@pcarruscag
Copy link
Member Author

Thanks @talbring. You were right about the includes, even for these classes with lots of inlines everything seems to work fine if only the "client" solver includes the classes it uses.

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.

3 participants