Conversation
original:
label:
stuff A
if (some condition) {
stuff B
goto label
}
intermediate step:
label:
stuff A
calculate condition before updating Ncomp
stuff B
if (some condition) {
goto label
}
final step:
do {
stuff A
stuff B
} while (condition)
original:
label:
stuff A
if (some condition) {
stuff B
goto label
}
intermediate step:
label:
stuff A
calculate condition before updating Ncomp
stuff B
if (some condition) {
goto label
}
final step:
do {
stuff A
calculate condition before updating Ncomp
stuff B
} while (condition)
…o refactorGOTO
|
|
||
| // Iteration_Ncomp: Perform iterations to calculate Ncomp (Label20) | ||
| Counter = 1; | ||
| Label20:; |
| CompSpdActual, | ||
| Ncomp_new); | ||
|
|
||
| bool not_converged = (std::abs(Ncomp_new - Ncomp) > (Tolerance * Ncomp)); |
There was a problem hiding this comment.
If you are still making changes in this PR, could I make the tiniest of requests? Could you make the variable name converged instead of not_converged? If we ever need to check if we are converged, we have to check !not_converged, and that's a more difficult mental parse, at least to me. If you are done touching this code, then feel free to ignore, it's not all that important :)
There was a problem hiding this comment.
Thanks for the suggestion @Myoldmopar. I will change it to converged instead. I will continue working on this.
assume the evaluation of conditions doesn't change code behavior
this evaluates to A(BA)*
label: <- the original code
stuff A
if (condition) {
stuff B
goto label
}
this evaluates to (AB)+
do {
stuff A
evaluate condition (in case stuff overwrites variables used in it)
stuff B
} while (condition)
This will lead to one more "B" than the original code
So change it to this
do {
stuff A
evaluate condition (in case stuff overwrites variables used in it)
if (condition) {
stuff B
}
} while (condition)
evaluated diffs from VariableRefrigerantFlow_FluidTCtrl_HR_5Zone.idf
The following seems to have the same output
label:
stuff A
evaluate condition (in case stuff overwrites variables used in it)
if (condition) {
stuff B
goto label
}
vs
label:
stuff A
evaluate condition (in case stuff overwrites variables used in it)
stuff B
if (condition) {
goto label
}
there's only a counter increase inside the if condition in goto
|
@yujiex @Myoldmopar it has been 28 days since this pull request was last updated. |
3 similar comments
|
@yujiex @Myoldmopar it has been 28 days since this pull request was last updated. |
|
@yujiex @Myoldmopar it has been 28 days since this pull request was last updated. |
|
@yujiex @Myoldmopar it has been 28 days since this pull request was last updated. |
|
@yujiex @Myoldmopar it has been 28 days since this pull request was last updated. |
1 similar comment
|
@yujiex @Myoldmopar it has been 28 days since this pull request was last updated. |
|
@yujiex it has been 20 days since this pull request was last updated. |
|
@yujiex it has been 7 days since this pull request was last updated. |
1 similar comment
|
@yujiex it has been 7 days since this pull request was last updated. |
|
@yujiex it has been 7 days since this pull request was last updated. |
2 similar comments
|
@yujiex it has been 7 days since this pull request was last updated. |
|
@yujiex it has been 7 days since this pull request was last updated. |
|
@yujiex it has been 8 days since this pull request was last updated. |
|
@yujiex it has been 7 days since this pull request was last updated. |
2 similar comments
|
@yujiex it has been 7 days since this pull request was last updated. |
|
@yujiex it has been 7 days since this pull request was last updated. |
|
This is great. One wish is that while in here, the 1300 line function for CalcVRF or whatever it's called could have been split into a few meaningful functions. But that's fine. I'm not sure why the rest of CI didn't run here, but I suspect when the curve API branch merges, there could be a conflict or two to resolve. I'll tackle it and let you know if I have any issues. |
|
Wow, no conflicts. OK, I'm still gonna pull in the latest develop for a quick sanity check. |
|
Alright, if this comes back clean, it'll merge in next. |
|
All good here, thanks @yujiex , merging this. |
|
Thanks @Myoldmopar ! |
Pull request overview
pattern 1
The following table shows some pseudo-code of the current goto example in the physics-based VRF model (column 1) and standard while and do-while loops (columns 2 and 3). To simplify the reasoning of its execution, I simplified the operations into letters and will check what type of regex they each produce.
A = stuff A, B = stuff B and counter increment, C being the condition check
For this pattern of existing code. I'll try to hoist the "stuff B" part out of the if branch and have an intermediate step
Then this can be converted to a do-while loop
suppose D = evaluate condition (in case stuff overwrites variables used in it), then both the intermediate and final step evaluates to (ADBC)+
If A and B are the things that matter most, than the original version of code will execute to A(BA)*, and the new code will execute to (AB)+, with a trailing B comparing with the original code. If this additional B doesn't impact the result, then this could be a way to transition such a pattern of goto to a standard while loop.
Otherwise, the following should be used, which evaluates to ADC(BCADC)*. If D and C doesn't change the behavior, then it evaluates to A(BA)*
pattern 2
goto jumps out two or more levels, like in label23 in
HVACVariableRefrigerantFlow.cc.This can be converted to a do-while loop as follows
In both cases, the following is what it evaluates to for different combination of C and D
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.