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

Reactor: ensure consistent vector lengths in C++ layer #756

Merged
merged 2 commits into from
Nov 28, 2019

Conversation

ischoegl
Copy link
Member

@ischoegl ischoegl commented Nov 20, 2019

Changes proposed in this pull request

  • Ensure consistent vector sizes, i.e. Reactor::m_advancelimits.size() is Reactor::m_nv
  • Prevents hard to track segfaults when m_advancelimits.size() < m_nv in custom additions
  • Make functions related to advance limits non-virtual

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.

I see the issue with the keeping the length of m_advancelimits consistent across different derived classes (including user-created ones). I wanted to propose an alternative solution, which is to simply resize it to the correct size in the few locations where it is used (all of which are within the Reactor base class). See bd58afa for the implementation of this option.

I'm 👍 on making the related methods non-virtual.

@ischoegl
Copy link
Member Author

ischoegl commented Nov 27, 2019

@speth ... resizing at a later time is a very good suggestion! Not having to resize in overloaded functions is great. Along those lines, I am suggesting to not resize m_advancelimits at all unless it is actually being used. (which allows for simple checks that reduce overhead).

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.

This looks good to me. I only have a couple of minor suggestions. In addition to these, can you squash the third commit into the first, since it reverts a number of the changes made in the first commit?

src/zeroD/Reactor.cpp Outdated Show resolved Hide resolved
src/zeroD/Reactor.cpp Outdated Show resolved Hide resolved
src/zeroD/Reactor.cpp Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Nov 28, 2019

Codecov Report

Merging #756 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #756      +/-   ##
==========================================
+ Coverage   70.83%   70.84%   +<.01%     
==========================================
  Files         372      372              
  Lines       43720    43732      +12     
==========================================
+ Hits        30971    30982      +11     
- Misses      12749    12750       +1
Impacted Files Coverage Δ
include/cantera/zeroD/ReactorNet.h 80% <ø> (ø) ⬆️
src/zeroD/Reactor.cpp 84.81% <100%> (+0.28%) ⬆️
include/cantera/zeroD/Reactor.h 69.23% <100%> (+2.56%) ⬆️
src/zeroD/ReactorNet.cpp 84.02% <100%> (+0.42%) ⬆️
src/transport/GasTransport.cpp 90.58% <0%> (-0.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba330a7...52e7920. Read the comment docs.

@ischoegl
Copy link
Member Author

Done (and squashed).

@ischoegl ischoegl requested a review from speth November 28, 2019 02:41
@speth speth merged commit 46fbdc4 into Cantera:master Nov 28, 2019
@ischoegl ischoegl deleted the consistent-limit-length branch December 16, 2019 15:55
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.

2 participants