Skip to content

CatchmentCsvOutputManager: Avoid hash<std::filessytem::path> compatibility pitfall - #1015

Open
PhilMiller wants to merge 1 commit into
NOAA-OWP:masterfrom
PhilMiller:PhilMiller/std-fs-hash-fix
Open

CatchmentCsvOutputManager: Avoid hash<std::filessytem::path> compatibility pitfall#1015
PhilMiller wants to merge 1 commit into
NOAA-OWP:masterfrom
PhilMiller:PhilMiller/std-fs-hash-fix

Conversation

@PhilMiller

@PhilMiller PhilMiller commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Switch use of an unordered_map<std::filesystem::path, stream*> to map<path, stream*> to avoid the need for a hash function for std::filesystem::path. This is an issue because C++17 inadvertently omitted that requirement, and compilers' standard libraries conforming to C++17 didn't implement it until several versions later. For instance, GCC 9 supports full C++17, but the hash isn't implemented until 11.4.0, and is missing even in 11.2.0.

Cf:
https://cplusplus.github.io/LWG/issue3657
And the very last table row here:
https://en.cppreference.com/cpp/compiler_support/17

Testing

  1. Local build
  2. CI

Notes

  • I think the hypothetical performance impact of this should be negligible, especially since we should move away from CSV-per-catchment output when we care about performance anyway.

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

…ility pitfall

Switch use of an unordered_map<std::filesystem::path, stream*> to
map<path, stream*> to avoid the need for a hash function for
std::filesystem::path. This is an issue because C++17 inadvertently
omitted that requirement, and compilers' standard libraries conforming
to C++17 didn't implement it until several versions later. For
instance, GCC 9 supports full C++17, but the hash isn't implemented
until 11.4.0, and is missing even in 11.2.0.

Cf:
https://cplusplus.github.io/LWG/issue3657
And the very last table row here:
https://en.cppreference.com/cpp/compiler_support/17
@PhilMiller

Copy link
Copy Markdown
Contributor Author

I may also simplify out the corresponding bits in the Docker file and the dependencies documentation.

@PhilMiller

Copy link
Copy Markdown
Contributor Author

Ahmad ran into this on the NSF Anvil cluster, which provides the not-good-enough GCC 11.2.0.

@hellkite500

Copy link
Copy Markdown
Contributor

I may also simplify out the corresponding bits in the Docker file and the dependencies documentation.

We should update these to drop the note about the hash limitation since it is no longer a blocker. GCC 9 still becomes the minimum for the std::filesystem implementation, and I don't see any real performance considerations that are going to matter for the typical use of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants