-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Move clib-demo to samples #1454
Conversation
27390fe
to
aded528
Compare
Codecov Report
@@ Coverage Diff @@
## main #1454 +/- ##
=======================================
Coverage 69.63% 69.64%
=======================================
Files 373 374 +1
Lines 55769 55812 +43
Branches 18295 18298 +3
=======================================
+ Hits 38833 38868 +35
- Misses 14480 14487 +7
- Partials 2456 2457 +1
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
293ea14
to
f9d45ff
Compare
The updated unit test works, but I'm not sure why the substituted |
I tested this, and they do get generated when you run the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @ischoegl. I think this change generally makes sense, and installing a pure clib
example is useful.
One thing that's lost here is what was described at the top of clib_test.c
:
// Include all clib headers to make sure all of them are C-compatible, even if
// we don't actually use all of them in this test.
#include "cantera/clib/ct.h"
#include "cantera/clib/ctfunc.h"
#include "cantera/clib/ctmultiphase.h"
#include "cantera/clib/ctonedim.h"
#include "cantera/clib/ctreactor.h"
#include "cantera/clib/ctrpath.h"
#include "cantera/clib/ctsurf.h"
The point was that this is the only file within Cantera that actually gets compiled by the C (rather than C++) compiler, and ensures that these files don't do anything bad like include a C++ header or syntax that isn't valid C (which was in fact the case before I originally added this test). This is why this test was added to the older test_problems
test suite rather than the all-C++ gtest test suite, and I think we still need something that ensures this, even if it's not the previous version of the clib
test.
c338d8d
to
549a1b0
Compare
@speth ... thanks for the feedback and for shedding some light on the
Good point. I re-added the 'lost' includes to |
_MSC_VER < 1900 corresponds to 1800 and earlier, which is VC++ version 12.0 (Visual Studio 2013). Cantera now uses the C++17 standard, with the current minimum requirement VC++ version 14.1 (Visual Studio 2017).
549a1b0
to
4f519b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Changes proposed in this pull request
Detailed tests included in
clib_test.c
are superseded by the gtestclib
suite. Nevertheless, the original file contains a useful example. This PR moves a slightly updated example tosamples/clib
while retaining the output file comparison.If applicable, provide an example illustrating new features this pull request is introducing
The behavior of
is unchanged, but the source code is now compiled from
samples/clib/demo.c
.Checklist
scons build
&scons test
) and unit tests address code coverage