Skip to content

Take std::filesystem::path where opm-common opens a file - #5345

Draft
GitPaean wants to merge 3 commits into
OPM:masterfrom
GitPaean:path-parameters
Draft

Take std::filesystem::path where opm-common opens a file#5345
GitPaean wants to merge 3 commits into
OPM:masterfrom
GitPaean:path-parameters

Conversation

@GitPaean

@GitPaean GitPaean commented Sep 6, 2026

Copy link
Copy Markdown
Member
  1. Takes std::filesystem::path where opm-common opens a file — the EclIO readers and writers, Parser::parseFile(), and the keyword generator's file helpers — so a caller holding a path passes it as is. std::string still converts implicitly: no call site changes, and every consumer in opm-common, opm-grid and opm-simulators builds unchanged.
  2. Removes the conversions the library was doing to itself (EclFile(rstFile.string()), EclOutput(smryDataFile.generic_string(), …), EInit(initFileName.generic_string())) and retires the parser's dataFileName[0] == '/' in favour of has_root_directory(), the same test expressed on the path rather than on its first byte.
  3. No functional change: the names simply travel as the type the standard library opens files with, and nothing decides anything by inspecting their characters any more; the full opm-common test suite passes as before.

A path converts implicitly to std::string only where path::value_type
is char, so a caller holding a path had to spell .string() first, and
the library did the same to itself: EclFile(rstFile.string()),
EclOutput(smryDataFile.generic_string(), ...) and
EInit(initFileName.generic_string()). Take and store the path instead:
std::string still converts implicitly, so callers are unchanged, and
the streams open with the path directly.

Messages that concatenated the name go through fmt with <fmt/std.h>,
which prints the path quoted.
Same reason as for EclIO: a path converts implicitly to std::string
only where path::value_type is char. The absolute-path test becomes
has_root_directory(), the portable spelling of the former leading-'/'
test, so a rooted path is kept as given wherever it is not also
absolute. The deck-visible name stays a generic_string().

The Python binding keeps its std::string signature through lambdas so
it does not depend on pybind11's filesystem caster.
ensurePath(), updateFile() and the write_file() helpers take the path
their callers already build, instead of receiving it as std::string and
converting back. The update*() entry points keep std::string: they join
the pieces CMake passes by plain concatenation.
@GitPaean GitPaean added the manual:irrelevant This PR is a minor fix and should not appear in the manual label Sep 6, 2026
@GitPaean

GitPaean commented Sep 6, 2026

Copy link
Copy Markdown
Member Author

jenkins build this please

@GitPaean

GitPaean commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

I consider this is a small improvements arguably, I would like to get some input before continue further. @bska @atgeirr @akva2

@akva2 akva2 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense to me. First commit would benefit from a split (path and fmt parts)

@bska

bska commented Sep 7, 2026

Copy link
Copy Markdown
Member

I concur with @akva2: Taking path instead of string (or string_view) in those places that actually treat the string as a path name is the correct thing to do. I'd also prefer that fmt-related changes be split into separate commits, possibly even separate PRs.

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

Labels

manual:irrelevant This PR is a minor fix and should not appear in the manual

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants