Reduce Optional_string usage #9232

jasondegraw · 2022-01-12T20:13:21Z

Pull request overview

Reduce the usage of the various forms of string optional arguments by about 50%. In many cases, the optional can be replaced with a std::string_view and the presence of the string can be checked with the empty method. Most of the remaining cases either require a deeper refactor or are too tied up with other ongoing refactors that there's not a good way to get at changing just the optional argument. In a few situations, overloading has been used to shift run-time choices of behavior to compile-time choices of behavior, so there may be a (very) small performance benefit from this PR.

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
Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
If changes fix a defect, the fix should be demonstrated in plots and descriptions

Reviewer

This will not be exhaustively relevant to every PR.

…into optional-string

jasondegraw · 2022-01-12T20:17:15Z

src/EnergyPlus/DXCoils.cc

@@ -15687,8 +15687,8 @@ void GetDXCoilIndex(EnergyPlusData &state,
                    std::string const &DXCoilName,
                    int &DXCoilIndex,
                    bool &ErrorsFound,
-                    Optional_string_const ThisObjectType,
-                    Optional_bool_const SuppressWarning)
+                    std::string_view const ThisObjectType,


This one is a good example of the easier changes. The argument is only used for errors, so the argument type is changed first.

jasondegraw · 2022-01-12T20:18:05Z

src/EnergyPlus/DXCoils.cc

-        } else {
-            if (present(ThisObjectType)) {
-                ShowSevereError(state, ThisObjectType + ", GetDXCoilIndex: DX Coil not found=" + DXCoilName);
+            if (!ThisObjectType.empty()) {


Then the call to present is changed to use the empty method.

jasondegraw · 2022-01-12T20:18:53Z

src/EnergyPlus/DXCoils.cc

-            if (present(ThisObjectType)) {
-                ShowSevereError(state, ThisObjectType + ", GetDXCoilIndex: DX Coil not found=" + DXCoilName);
+            if (!ThisObjectType.empty()) {
+                ShowSevereError(state, fmt::format("{}, GetDXCoilIndex: DX Coil not found={}", ThisObjectType, DXCoilName));


And finally the string handling is changed to something compatible with string_view.

jasondegraw · 2022-01-12T20:23:56Z

src/EnergyPlus/NodeInputManager.cc

-                      bool &ErrorsFound,
-                      Optional_string_const CheckName,
-                      Optional_int_const CheckNumber,
-                      Optional_string_const ObjectName)


This function was checking for uniqueness of an optional name or optional number based on the string in CheckType... which appears to always be one of two literal strings... so known at runtime. Replace that with two functions, one that checks for name and one that checks for number.

amirroth · 2022-01-12T21:54:11Z

src/EnergyPlus/DXCoils.cc

@@ -15720,7 +15719,7 @@ void GetDXCoilIndex(EnergyPlusData &state,
 }

 std::string
-GetDXCoilName(EnergyPlusData &state, int &DXCoilIndex, bool &ErrorsFound, Optional_string_const ThisObjectType, Optional_bool_const SuppressWarning)
+GetDXCoilName(EnergyPlusData &state, int &DXCoilIndex, bool &ErrorsFound, std::string_view const ThisObjectType, bool const SuppressWarning)


Functions like this should not be necessary. This should just be state.dataDXCoils->DXCoils(DXCoilNum).Name. The error checking and defaulting should not be necessary. You should not need to call a function to grab a field of an object whose index is known. I understand that this has nothing to do with this PR, but this is as good a place as any for me to hang this comment.

These functions should hopefully only get called during the input phase, but (a) good point and (b) I agree.

amirroth · 2022-01-12T21:54:41Z

src/EnergyPlus/DXCoils.hh


    std::string GetDXCoilName(
-        EnergyPlusData &state, int &DXCoilIndex, bool &ErrorsFound, Optional_string_const ThisObjectType, Optional_bool_const SuppressWarning);
+        EnergyPlusData &state, int &DXCoilIndex, bool &ErrorsFound, std::string_view const ThisObjectType = {}, bool const SuppressWarning = false);

    Real64 GetCoilCapacity(EnergyPlusData &state,


Same thing.

amirroth · 2022-01-12T21:55:03Z

src/EnergyPlus/DataSurfaceColors.hh

@@ -92,7 +92,7 @@ namespace DataSurfaceColors {
                                    std::string const &ColorType // for now, must be DXF
    );

-    void SetUpSchemeColors(EnergyPlusData &state, std::string const &SchemeName, Optional_string_const ColorType = _);
+    void SetUpSchemeColors(EnergyPlusData &state, std::string const &SchemeName, std::string const &ColorType);


std::string_view

amirroth · 2022-01-12T21:59:34Z

src/EnergyPlus/InputProcessing/InputProcessor.cc

@@ -1249,17 +1249,87 @@ int InputProcessor::getObjectItemNum(EnergyPlusData &state,
    return getIDFObjNum(state, ObjType, object_item_num); // if incoming input is idf, then return idf object order
 }

+void InputProcessor::lowerRangeCheck(EnergyPlusData &state,


General comment about functions like these. You are passing 9 parameters to this function, 8 of which are only needed in the case of an error, which is presumably rare. This is MCCS (Make the Common Case Slow) 101. If you want to make a function out of the error message code itself, okay. But do the range check at the call site and only call that function if there is an actual error.

I went ahead and refactored the function that got split into this one and the other one below, but this only gets used in WeatherManager regardless of whether it was intended to be a general use function. That's another argument for it to go.

Yeah. More important than this specific function, however, is the general lesson and pattern. One of the tenets of high-performance (or at least not terrible-performance) programming is "Make the Common Case Fast". Setting up parameters takes time. Don't take that time if those parameters will only be used infrequently, e.g., in the case of an error. Another example I see quite a bit of is functions that have many options that tweak their behavior in different ways. Yes, you are "reusing code" and that's good, but even good things can be taken too far. If those options are rarely used, then you're slowing down the more frequent instances of the function.

These "rules of thumb" are obvious once someone points them out to you. And they are invaluable. I learned this one in my second year graduate computer architecture class. MCCF is the rule of thumb for computer hardware design, and it applies to software just as well.

amirroth · 2022-01-12T22:00:08Z

src/EnergyPlus/InputProcessing/InputProcessor.cc

+        }
+    }
+}
+
 void InputProcessor::rangeCheck(EnergyPlusData &state,


Same comment as above. Understand that this is not part of this PR, but if I see something I'm going to say something.

amirroth · 2022-01-12T22:06:42Z

src/EnergyPlus/ScheduleManager.cc

-                                     Optional_string_const MaxString // Maximum indicator ('<', ',=')
+                                     int const ScheduleIndex, // Which Day Schedule being tested
+                                     Real64 const Minimum,    // Minimum desired value
+                                     bool const exclusiveMin, // Minimum indicator ('>', '>=')


amirroth · 2022-01-12T22:08:14Z

src/EnergyPlus/WeatherManager.cc

-                                                                              _,
-                                                                              _,
-                                                                              state.dataEnvrn->WeatherFileLocationTitle);
+                        state.dataInputProcessing->inputProcessor->lowerRangeCheck(state,


See comment above about rangeCheck function.

Myoldmopar · 2022-01-15T14:30:04Z

@jasondegraw the last PR just caused a conflict in one file. I'm happy to resolve if you want me to just let me know.

jasondegraw · 2022-01-15T15:32:09Z

@Myoldmopar Yes, that would be great if you could do that. This should be ready for review, when I last checked it was all good but it may have fallen behind develop a bit.

Myoldmopar · 2022-01-18T20:45:28Z

Conflicts resolved, everything builds and passes fine. Carry on...

jasondegraw · 2022-01-18T21:21:40Z

@Myoldmopar I think we want to get this in now and get out of the way of some of the other work that is going on, who should do review of this PR?

mitchute

Great to have these cleaned up and some of the messiness that came along with these optionals gone.

This all looks OK to me. Nothing is jumping out as controversial, just a nice set of cleanup changes. I think this can merge right away if CI comes back green.

Myoldmopar

Yeah, this looks awesome. I have no issues with this.

Myoldmopar · 2022-01-19T15:41:25Z

I'm not sure why Windows didn't come back yet, but I'm not going to hold it up. It was clean ahead of the merge from develop. Merging now, thanks @jasondegraw

Reduce Optional_string usage

jasondegraw added 9 commits January 10, 2022 00:14

Get the Optional_strings in Fans.cc and .hh

8183874

Minor reformat

83efd71

Remove some optional strings in DXCoils

bd5a9b5

More work on optional strings

b56833f

Remover misc. string optionals

47da64f

Remove optional strings in output

fe06238

Merge branch 'optional-string' of https://github.com/NREL/EnergyPlus …

8f40a07

…into optional-string

Wrap up a few more of the easier cases

40e1996

Merge branch 'develop' into optional-string

06eedf8

jasondegraw added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Jan 12, 2022

jasondegraw commented Jan 12, 2022

View reviewed changes

jasondegraw added 2 commits January 12, 2022 16:10

clang format changes

788b2db

More format fixes

6fdd943

amirroth reviewed Jan 12, 2022

View reviewed changes

jasondegraw added 2 commits January 13, 2022 10:21

Change one string to string_view

5c07508

Correct conversion error

04503db

Merge develop; resolve conflicts

4a0936a

mitchute approved these changes Jan 18, 2022

View reviewed changes

Myoldmopar approved these changes Jan 19, 2022

View reviewed changes

Myoldmopar merged commit 2f3618a into develop Jan 19, 2022

Myoldmopar deleted the optional-string branch January 19, 2022 15:41

yujiex pushed a commit that referenced this pull request Jan 27, 2022

Merge pull request #9232 from NREL/optional-string

7235408

Reduce Optional_string usage

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce Optional_string usage #9232

Reduce Optional_string usage #9232

jasondegraw commented Jan 12, 2022

jasondegraw Jan 12, 2022

jasondegraw Jan 12, 2022

jasondegraw Jan 12, 2022

jasondegraw Jan 12, 2022

amirroth Jan 12, 2022

jasondegraw Jan 13, 2022

amirroth Jan 12, 2022

amirroth Jan 12, 2022

amirroth Jan 12, 2022

jasondegraw Jan 13, 2022

amirroth Jan 13, 2022

amirroth Jan 12, 2022

amirroth Jan 12, 2022

amirroth Jan 12, 2022

Myoldmopar commented Jan 15, 2022

jasondegraw commented Jan 15, 2022

Myoldmopar commented Jan 18, 2022

jasondegraw commented Jan 18, 2022

mitchute left a comment

Myoldmopar left a comment

Myoldmopar commented Jan 19, 2022

Reduce Optional_string usage #9232

Reduce Optional_string usage #9232

Conversation

jasondegraw commented Jan 12, 2022

Pull request overview

Pull Request Author

Reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Myoldmopar commented Jan 15, 2022

jasondegraw commented Jan 15, 2022

Myoldmopar commented Jan 18, 2022

jasondegraw commented Jan 18, 2022

mitchute left a comment

Choose a reason for hiding this comment

Myoldmopar left a comment

Choose a reason for hiding this comment

Myoldmopar commented Jan 19, 2022