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
Fixed Crash in CondFD for Interzone Surfaces for Very Specific Condition #8937
Conversation
This commit includes the fix and the unit test for defect #8919. Under a particular condition (CondFD used, interzone surface that has another zone as the outside boundary condition causing E+ to create it internally, and the user supplying the reversed construction but it isn't used by any user defined surface), a variable was not being set (IsUsed) for the reversed construction causing a crash (array index of zero). This is now fixed and this particular case trapped by the new unit test.
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.
All good here. I'll test this out and get it in the merge queue.
@@ -906,6 +906,7 @@ int AssignReverseConstructionNumber(EnergyPlusData &state, | |||
} | |||
if (Found) { | |||
NewConstrNum = Loop; | |||
state.dataConstruction->Construct(Loop).IsUsed = true; |
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.
Seems quite reasonable.
// for other defined surfaces, IsUsed was set to false and this construction | ||
// was then skipped in the CondFD routine that calculates the number of | ||
// nodes. This led to a hard crash. This test simply makes sure that | ||
// IsUsed is set to true when the reversed construction already exists. |
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.
This helps me understand not only the fix you made, but also the intent of the unit test. This is good stuff. Thanks @RKStrand
int ConstrNum; | ||
int expectedResultRevConstrNum; | ||
int functionResultRevConstrNum; | ||
bool ErrorsFound = false; |
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.
Don't forget, in C++, you don't have to declare the variables up here at the top of the function, just declare them at the point you use them. 👍
The defect file was apparently sent by email and not attached to the issue or this PR or put in DevSupport, so I cannot verify the fix, but I'm trusting that it has been fixed and will use the new unit test as verification for now. Building and running... |
All passing here. Thanks @RKStrand |
Pull request overview
NOTE: ENHANCEMENTS MUST FOLLOW A SUBMISSION PROCESS INCLUDING A FEATURE PROPOSAL AND DESIGN DOCUMENT PRIOR TO SUBMITTING CODE
Pull Request Author
Add to this list or remove from it as applicable. This is a simple templated set of guidelines.
Reviewer
This will not be exhaustively relevant to every PR.