Update ssz zsz mtr files to fmt #7804
Conversation
…zsz_mtr_files_to_fmt
lefticus
added
the
Refactoring
| @@ -240,13 +240,8 @@ namespace DataGlobals { | |||
| extern int OutputStandardError; // Unit number for the standard error output file | |||
| extern std::ostream *err_stream; // Internal stream used for err output (used for performance) | |||
| extern int StdOutputRecordCount; // Count of Standard output records | |||
| extern std::ostream *eio_stream; // Internal stream used for eio output (used for unit tests) | |||
| std::string outputZszCsvFileName("epluszsz.csv"); | ||
| std::string outputZszTabFileName("epluszsz.tab"); | ||
| std::string outputZszTxtFileName("epluszsz.txt"); | ||
| std::string outputSszCsvFileName("eplusssz.csv"); | ||
| std::string outputSszTabFileName("eplusssz.tab"); | ||
| std::string outputSszTxtFileName("eplusssz.txt"); |
|
No issues that I can see here. CI looks good and unit tests pass locally. One minor quibble: There are a few relative path references ( @Myoldmopar anything else before I merge it? |
There was a problem hiding this comment.
This is a good one. This looks to be spreading the output files instance pretty far around the code, which is a great step. It should reduce the amount of code changes for future file changes.
I will lean pretty heavily on CI results here, but I'll also pull and build and do some spot testing as well right now.
| static ObjexxFCL::gio::Fmt EnvironmentStampFormat("(a,',',a,3(',',f7.2),',',f7.2)"); // Format descriptor for environ stamp | ||
| static constexpr auto EnvironmentStampFormatStr("{},{},{:7.2F},{:7.2F},{:7.2F},{:7.2F}\n"); // Format descriptor for environ stamp |
There was a problem hiding this comment.
@lefticus is there a gain to using auto here? I like using it for references so that we don't have to spell out a long type name, but in cases like this, I kinda like being intentional. It should just be a std::string, right?
This doesn't need to be fixed before this PR merges, but I'd like to understand if there is an alternative benefit from using auto before it is used more widespread.
There was a problem hiding this comment.
@Myoldmopar Interesting one for you to bring up. The actual type is const char *, which is why I didn't spell it out.
using std::string for the type would actually cause a performance hit because it cannot be constexpr and would cause a static initialization check on each access to the value.
For the moment I'm leaving them as auto for place holders with the hope that if we upgrade the build system to C++14 or greater (I haven't done that yet) I can use constxpr fmt::string_view which means the option of some compile-time checking of parameters as well as better perf at runtime.
There was a problem hiding this comment.
That makes sense, thanks for the clarification. As for moving to C++14, Linux and Mac should be no problem, right? Is there a specific version of Visual Studio we need to target?
There was a problem hiding this comment.
@Myoldmopar 2019 would be amazing, 2017 would be awesome. But in general I think it's time for a refresh to set the minimum supported compilers so we can move forward in places where it makes sense to, even with gcc/clang.
|
Built locally and ran a few tests. At first I thought I identified a problem in the zsz file because there was a blank line near the end with this branch. But then I ran develop and found it is also there. Manually diff-ing the zsz, ssz, and mtr files showed no meaningful diffs, just the E+ version header in the mtr. This is good to go in. @mitchute merge at will. |
I have started doing these manual diffs before pushing to reduce the potential friction. I find it only takes a few more minutes on my side and saves hours of waiting for CI / reviews. |
Pull request overview