Avoid using DataStringConstants::NL whenever possible and trust OS#8178
Avoid using DataStringConstants::NL whenever possible and trust OS#8178Myoldmopar merged 3 commits intodevelopfrom
Conversation
- On windows, with a text file, '\n' does the right thing
Myoldmopar
left a comment
There was a problem hiding this comment.
I haven't tested this, but the changes look top notch.
|
|
||
| if (UtilityRoutines::outputErrorHeader && err_stream) { | ||
| *err_stream << "Program Version," + VerString + ',' + IDDVerString + DataStringGlobals::NL; | ||
| *err_stream << "Program Version," << VerString << ',' << IDDVerString << '\n'; |
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh I missed your "not" being a link :D
| namespace ObjexxFCL { | ||
|
|
||
| extern char const CMA; | ||
| extern char const NL; |
There was a problem hiding this comment.
Nice nice nice. One less to manage.
mjwitte
left a comment
There was a problem hiding this comment.
Tested locally on windows. All good now.
|
But I did not run unit tests locally...
|
|
@mjwitte All those uses of |
|
@mjwitte it looks like you are saying there is still a step to be made to fix up the unit tests, is that still the case here? |
|
@Myoldmopar Yes, this still needs more work. See the lastest Windows CI results |
|
OK, I can tackle that cleanup. I built and ran it on Linux but the unit tests run fine. I'll need to build and run on Windows to see it, which isn't a big problem. I'll try to do that soon. Unless @lefticus is already making these changes... |
|
@Myoldmopar The changes seem reasonable and the unit tests seem to pass. |
|
@mjwitte do you want to re-run with my fix? It looks like it is good to go to me. |
|
Sorry, missed this ongoing conversation @Myoldmopar the changes you made are inline with similar changes I've had to make as I've updated each output file to being in text mode instead of binary mode. |
|
@Myoldmopar I've re-run with this branch. Output files look good. Converted between idf and epJSON in both directions and the converted files look good. Other than the failed movefile_test on windows this is good to go. I'm assuming that's a random test failure. The test passes locally. |
Very simple PR. Should specifically fix the issues that @mjwitte noticed for error output files.