Skip to content

refactoring goto statements#10505

Merged
Myoldmopar merged 28 commits intodevelopfrom
refactorGOTO
Apr 10, 2025
Merged

refactoring goto statements#10505
Myoldmopar merged 28 commits intodevelopfrom
refactorGOTO

Conversation

@yujiex
Copy link
Collaborator

@yujiex yujiex commented May 15, 2024

Pull request overview

  • This PR aims to convert the goto statements in the physics-based VRF model to use more standard while or do-while loops.

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.

image

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

    label:
        stuff A
        evaluate condition (in case stuff overwrites variables used in it)
        stuff B
    if (condition) {
        goto label
    }

Then this can be converted to a do-while loop

    do {
        stuff A
        evaluate condition (in case stuff overwrites variables used in it)
        stuff B
    } while (condition)

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)*

    do {
        stuff A
        evaluate condition (in case stuff overwrites variables used in it)
        if (condition) {
            stuff B
        }
    } while (condition)

pattern 2

goto jumps out two or more levels, like in label23 in HVACVariableRefrigerantFlow.cc.

Label:;
stuff A
if (C) {
    stuff E
} else {
    stuff F
    if (D) {
        stuff B
        goto label
    }
    stuff G
}

This can be converted to a do-while loop as follows

do {
    stuff A
    if (C) {
        stuff E
        break
    } else {
        stuff F
        if (D) {
            stuff B
        } else {
            stuff G
            break
        }
    }
} while (D)

In both cases, the following is what it evaluates to for different combination of C and D

C true  D true:  ACE
C true  D false: ACE
C false D true:  (ACFDB)+ACFDG
C false D false: ACFDG

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.

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

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)
@yujiex yujiex added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label May 15, 2024
@yujiex yujiex self-assigned this May 15, 2024
Yujie Xu added 2 commits May 15, 2024 14:46
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)
@yujiex yujiex marked this pull request as draft May 15, 2024 21:52

// Iteration_Ncomp: Perform iterations to calculate Ncomp (Label20)
Counter = 1;
Label20:;
Copy link
Collaborator

Choose a reason for hiding this comment

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

🙏

CompSpdActual,
Ncomp_new);

bool not_converged = (std::abs(Ncomp_new - Ncomp) > (Tolerance * Ncomp));
Copy link
Member

Choose a reason for hiding this comment

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

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 :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the suggestion @Myoldmopar. I will change it to converged instead. I will continue working on this.

Yujie Xu added 12 commits May 20, 2024 15:56
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
@nrel-bot
Copy link

@yujiex @Myoldmopar it has been 28 days since this pull request was last updated.

3 similar comments
@nrel-bot
Copy link

@yujiex @Myoldmopar it has been 28 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@yujiex @Myoldmopar it has been 28 days since this pull request was last updated.

@nrel-bot
Copy link

@yujiex @Myoldmopar it has been 28 days since this pull request was last updated.

@dareumnam dareumnam added this to the EnergyPlus 25.1 milestone Sep 6, 2024
@nrel-bot-2c
Copy link

@yujiex @Myoldmopar it has been 28 days since this pull request was last updated.

1 similar comment
@nrel-bot-2c
Copy link

@yujiex @Myoldmopar it has been 28 days since this pull request was last updated.

@nrel-bot-2
Copy link

@yujiex it has been 20 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@yujiex it has been 7 days since this pull request was last updated.

1 similar comment
@nrel-bot-2c
Copy link

@yujiex it has been 7 days since this pull request was last updated.

@nrel-bot-2
Copy link

@yujiex it has been 7 days since this pull request was last updated.

2 similar comments
@nrel-bot-2c
Copy link

@yujiex it has been 7 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@yujiex it has been 7 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@yujiex it has been 8 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@yujiex it has been 7 days since this pull request was last updated.

2 similar comments
@nrel-bot-2c
Copy link

@yujiex it has been 7 days since this pull request was last updated.

@nrel-bot-2c
Copy link

@yujiex it has been 7 days since this pull request was last updated.

@yujiex yujiex marked this pull request as ready for review April 10, 2025 00:01
@Myoldmopar
Copy link
Member

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.

@Myoldmopar
Copy link
Member

Wow, no conflicts. OK, I'm still gonna pull in the latest develop for a quick sanity check.

@Myoldmopar
Copy link
Member

Alright, if this comes back clean, it'll merge in next.

@Myoldmopar
Copy link
Member

All good here, thanks @yujiex , merging this.

@Myoldmopar Myoldmopar merged commit 842796b into develop Apr 10, 2025
9 checks passed
@Myoldmopar Myoldmopar deleted the refactorGOTO branch April 10, 2025 15:18
@yujiex
Copy link
Collaborator Author

yujiex commented Apr 10, 2025

Thanks @Myoldmopar !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants