search

NREL / EnergyPlus

EnergyPlus™ is a whole building energy simulation program that engineers, architects, and researchers use to model both energy consumption and water use in buildings.
https://energyplus.net
Other
1.13k stars 391 forks source link

Time to remove the fortran number formats workarounds for fmt? #10596

Open jmarrec opened 3 months ago

jmarrec commented 3 months ago

Issue overview

Upgrading fmt from the current version of 8.0.1 to 10+ will require significant work to get the custom double formats to match the old fortran formats (Z, S, N, R).

I'm speaking about what is done around here: https://github.com/NREL/EnergyPlus/blob/1033bdc61ccda944a6844344f9ba18e76ffa53d1/src/EnergyPlus/IOFiles.hh#L95

It's not impossible to do it, but since (I believe) these were provided so the I/O upgrade to fmt could pass without CI diffs (so we could trust the upgrade), maybe it's time to just remove these formats, and accept the diffs?

Notes:

  1. Why upgrade fmt?
    • Not saying this needs to be done now, but just anticipating that at some point we will want to do it.
    • it seems like this is going to be required to bring the codebase to C++20 and above (from my limited testing enabling C++20 instead of C++17).
    • And recent fmt versions gained interesting functionality like formatting std::filesystem::path out of the box, etc.
  2. The reason there is work to be done is that fmt internals changed, and the specs_.type is no longer a char but an actual enum, so rewritting it requires more effort.

Details

Some additional details for this issue (if relevant):

Checklist

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

Myoldmopar commented 3 months ago

Yes, this is exactly right. With the scale and importance of converting the IO work, @lefticus spent major time ensuring we didn't accidentally/quietly break the outputs. So there is a lot of extra formatting/weight that is done. It was always intended that we would pull that back off in small chunks which will cause diffs, but they can be within the context of tiny little PRs. This is great.

lefticus commented 3 months ago

Even though I'm just lurking, I would love to see that compatibility stuff torn out. It's not free, and it does slow down the compilation and runtime compared to just using the built in formatters. It would be a significant effort to verify the output isn't changed in a meaningful way, however.