Global DataSystemVariables+FaultsManager

[jmythms]

Move global variables in DataSystemVariables.* and FaultsManager.* to state

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

[jmythms]

Sorry to keep beating on this, but should these not be constexpr? Or static constexpr? I believe that as they are, they will generate initialization code (maybe not). I was not against using constexpr, just auto.

No worries, thank you for noticing. I learned from this and will get it changed.

[amirroth]

No worries yourself. I only learned about constexpr two or three months ago (my active C++ programming years were 1995 to 2006) and I am still learning about it (I didn't know that a constexpr function can do partial reductions if only some of the arguments are constexpr until a few weeks ago). I think constexpr is very powerful and used properly can eliminate runtime code and reduce the amount of stuff we have to carry in the state object. I want to make sure that we are establishing these patterns.

[jmythms]

Thanks, I understand one of the primary advantages of using C++ is maintaining/increasing performance, and that we have to go a bit beyond basic C++ programming to try to get the most optimized code. It's been a wonderful experience to learn by doing and to learn from experienced programmers.

[Myoldmopar]

This one looks clean. The windows build error is a very spurious warning about a file being in use, and the linux regression is the gpu file that still randomly shows diffs. I'll take a quick look at the code changes, but mostly going to be relying on CI to validate these differences.

[Myoldmopar]

I do wonder how many (if any) of these are needed outside of the worker function(s) below. That's for another day, now that these are constexpr and not a problem anymore.

[Myoldmopar]

Yeah this is all good. Thanks @jmythms !

[jmythms]

Thank you!