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

refactoring goto statements #10505

Draft
wants to merge 15 commits into
base: develop
Choose a base branch
from
Draft

refactoring goto statements #10505

wants to merge 15 commits into from

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:;
      A
      if (C) {
        E
      } else {
        F
        if (D) {
          B
          goto label
        }
      }

not sure how to convert this

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
@@ -11687,36 +11687,36 @@ void VRFCondenserEquipment::CalcVRFCondenser_FluidTCtrl(EnergyPlusData &state)

// 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
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.

None yet

8 participants