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

Remove round trim cleanup formatting #8379

Merged
merged 8 commits into from
Nov 21, 2020

Conversation

lefticus
Copy link
Contributor

Pull request overview

Refactoring to remove RoundSigDigits and TrimSigDigits while adding format() calls where reasonable.

expected failure:

  • 1 regression test is now failing due to a copy-paste-error correction

No other changes expected.

* Remove the RoundSigDigits and TrimSigDigits functions
  * They were over used and misused for integer->string conversions
* Merge any string concatenations that used Round/Trim functions
  into libfmt operations
* Overall binary is ~2% smaller
* Diff set still needs to be formatted
@lefticus lefticus added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Nov 13, 2020
@lefticus lefticus requested a review from Myoldmopar November 13, 2020 05:18
@Myoldmopar
Copy link
Member

This is going to be great. I'll keep an eye on it for when CI is happy with it and you feel like you are done with it.

@lefticus
Copy link
Contributor Author

@Myoldmopar I'm clearly quite confused on this as of course I've been building it all along on my systems and it's not building at all on any CI. I'll see what's going on.

@Myoldmopar
Copy link
Member

Definitely closer, but Windows is still having trouble with an ambiguous fmt symbol. Once that is resolved I'll take a final look here.

@Myoldmopar
Copy link
Member

I'll resolve the conflicts here and get this ready to go. Thanks for fixing the fmt issue @lefticus.

@lefticus
Copy link
Contributor Author

Sorry, I missed the conflicts. I headed out of town for a couple of days

@mitchute
Copy link
Collaborator

I'm not sure why CI complained on the unit test coverage build, but the AFN error message diff is definitely fixing an erroneous error message here: https://github.com/NREL/EnergyPlus/blob/develop/src/EnergyPlus/AirflowNetwork/src/Properties.cpp#L85

We'll keep an eye on CI for this next round of results and go from there.

@lefticus
Copy link
Contributor Author

I'm not sure why CI complained on the unit test coverage build, but the AFN error message diff is definitely fixing an erroneous error message here: https://github.com/NREL/EnergyPlus/blob/develop/src/EnergyPlus/AirflowNetwork/src/Properties.cpp#L85

Yes, I saw many minor mistakes during the conversion that I left because I didn't want to cause too many diffs, but when I saw this mistake which was 100% wrong, I felt I needed to fix it (it's mentioned in the PR also)

@mitchute
Copy link
Collaborator

Thanks, @lefticus. This is looking really good. I'll try to get this one in right away so we don't keep having to deal with merge conflicts.

Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice to see these cleaned up!

@Myoldmopar
Copy link
Member

I see the coverage dipped very slightly because of lines changed here. I don't anticipate artificially pumping that back up or adjusting the limit, we'll leave the warning and try to bump it back up through other PRs. This looks otherwise good on CI with the one expected diff and the GPU spurious regressions. I see this is ready and will merge, thanks!

@Myoldmopar Myoldmopar merged commit a047218 into develop Nov 21, 2020
@Myoldmopar Myoldmopar deleted the remove_round_trim_cleanup_formatting branch November 21, 2020 13:48
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.

8 participants