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

Add RFC 105 text: Add and use safe path manipulation functions #11640

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

rouault
Copy link
Member

@rouault rouault commented Jan 12, 2025

@rouault rouault added this to the 3.11.0 milestone Jan 12, 2025
@rouault rouault marked this pull request as draft January 12, 2025 14:47
@rouault rouault force-pushed the rfc105_text branch 2 times, most recently from 9957c92 to 67631c8 Compare January 12, 2025 16:38
@rouault rouault added the funded through GSP Work funded through the GDAL Sponsorship Program label Jan 12, 2025
Co-authored-by: Laurențiu Nicola <[email protected]>
@jjimenezshaw
Copy link
Contributor

Thank you @rouault . Great analysis.

@lnicola
Copy link
Contributor

lnicola commented Jan 13, 2025

Are there plans to require C++17 "soon"? It has std::filesystem, which might be an alternative. There's also the question of non-ASCII file paths, but it's better left unasked (and I don't think this RFC makes support for them worse).

LGTM otherwise.

@rouault
Copy link
Member Author

rouault commented Jan 13, 2025

Are there plans to require C++17 "soon"?

This is already a requirement

It has std::filesystem, which might be an alternative

My ambition doesn't go that far. It would be a significantly more invasive change. It is likely that our custom 25+ year functions do very similar things than std::filesystem::path (https://en.cppreference.com/w/cpp/filesystem/path), but I'd be afraid there are "subtle" differences that would prevent automatic replacement.

There's also the question of non-ASCII file paths,

Our current methods require UTF-8 paths.

@hobu
Copy link
Contributor

hobu commented Jan 13, 2025

I also would like to get to a point where these CPL functions are simple wrappers of std::filesystem, but there are likely to be platform peculiarities that CPL is pasting over in ways that are not compatible. We should at least attempt to wrap std::filesystem implementations for all the CPL path handling stuff and run the test suite and see how much actually falls out before we build another big chunk of code that we have to maintain going forward...

I have a problem with the name "Safe", but no problem with the intention and proposed fixes. As a user of the function, of course I want to use "safe" things 😄

Maybe calling them CPLXXXXString is a bit more clear? Naming things is hard. CPLXXXX2? IDK. The safe moniker implies something scary about the older usage.

@jjimenezshaw
Copy link
Contributor

The safe moniker implies something scary about the older usage.

That's the idea of this RFC, right?

@rouault
Copy link
Member Author

rouault commented Jan 13, 2025

I also would like to get to a point where these CPL functions are simple wrappers of std::filesystem, but there are likely to be platform peculiarities that CPL is pasting over in ways that are not compatible.

Indeed... How std::filesystem on MSVC would work with /vsi paths... ?

We should at least attempt to wrap std::filesystem implementations for all the CPL path handling stuff and run the test suite and see how much actually falls out

Even doing the "dumb" replacement I suggest in the RFC is going to be very painful, because there are several hundreds of locations where the sed magic isn't powerful enough to automate it and that will require hand editing. I'll let std::filesystem exploration to someone braver than me :-)

before we build another big chunk of code that we have to maintain going forward...

The change of the CPLxxxx() functions to return std::string and no longer use C concatenation function was the easy part and it is not that much (300 hundred lines of code maybe?)

The safe moniker implies something scary about the older usage.

Well, they are somewhat scary to use, so better have people ran away from them :-) Joke aside, I'm fine with the "2" is there's a consensus it is better than "Safe". I also considerd a "Cpp" suffix to indicate they are for use by C++ code, but wasn't convinced. We have some precedent of using "2" suffix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
funded through GSP Work funded through the GDAL Sponsorship Program
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants