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

Update C++ tutorial #192

Merged
merged 3 commits into from
Apr 6, 2022
Merged

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Apr 4, 2022

  • Replace newPhase by newSolution in C++ examples
  • Add information on environment variables to C++ compilation tutorial

Closes #191

@ischoegl ischoegl requested a review from a team April 4, 2022 20:55
@ischoegl ischoegl force-pushed the update-very-simple-cxx-example branch 4 times, most recently from b8d0fce to d690ded Compare April 5, 2022 01:32
@ischoegl ischoegl requested a review from speth April 5, 2022 01:46
@ischoegl
Copy link
Member Author

ischoegl commented Apr 5, 2022

@speth ... ended up replacing newPhase in the remaining tutorials also, and fixed a couple of other things that were not up-to-date in the documentation. I tested compilation of all examples, and things work as expected.

@ischoegl ischoegl force-pushed the update-very-simple-cxx-example branch 2 times, most recently from 725c735 to 0e14050 Compare April 5, 2022 02:45
@ischoegl
Copy link
Member Author

ischoegl commented Apr 5, 2022

Fwiw, some of the includes can be simplified if Cantera/cantera#1238 were to be adopted.

@bryanwweber
Copy link
Member

As @speth mentioned elsewhere, I don't think now is a good time to get into a conversation about the content of the include files. I'd prefer not dropping the lowercase include files from the examples.

I think we should limit this PR to the changes in the export and library configuration, and defer changing the examples until later. Since the website can be updated independently of a release, we can change these to newSolution later.

@ischoegl
Copy link
Member Author

ischoegl commented Apr 5, 2022

@bryanwweber ... thank you for the comments.

As @speth mentioned elsewhere, I don't think now is a good time to get into a conversation about the content of the include files. I'd prefer not dropping the lowercase include files from the examples.

I'm totally fine with keeping 'convenience' headers. At the same time, not having Solution.h covered is not great.

I think we should limit this PR to the changes in the export and library configuration, and defer changing the examples until later. Since the website can be updated independently of a release, we can change these to newSolution later.

I'm not sure I agree here: newSolution pretty much replaced most of the instances in the C++ test suite already, and is overall much more convenient for the users to use (see factory_demo.cpp!).

My preferred solution would be to switch the imports to

#include "cantera/core.h"

as proposed in PR Cantera/cantera#1238 (will wait on feedback there).

PS: If Cantera/cantera#1238 goes ahead, I'd redirect this PR to testing.

@ischoegl ischoegl marked this pull request as draft April 5, 2022 21:44
@ischoegl ischoegl force-pushed the update-very-simple-cxx-example branch from 0e14050 to 9f13360 Compare April 6, 2022 12:25
@ischoegl ischoegl force-pushed the update-very-simple-cxx-example branch from 9f13360 to fbf540f Compare April 6, 2022 12:36
@ischoegl
Copy link
Member Author

ischoegl commented Apr 6, 2022

@bryanwweber and @speth ... now that cantera/core.h is deferred (PR Cantera/cantera#1238), I updated the examples so they are up to date for the 2.6 release (and also work for 2.5.1). This is ready for another round of reviews.

@ischoegl ischoegl marked this pull request as ready for review April 6, 2022 12:40
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Thanks, @ischoegl. This looks good to me.

@speth speth merged commit eda140f into Cantera:main Apr 6, 2022
@ischoegl ischoegl deleted the update-very-simple-cxx-example branch April 6, 2022 14:41
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.

Update C++ tutorial and very simple C++ program
3 participants