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

Use std compliant non-method std::filesystem::exists function #1502

Merged
merged 1 commit into from
Jan 5, 2021

Conversation

jdlangs
Copy link
Contributor

@jdlangs jdlangs commented Jan 2, 2021

I was looking into updating rcpputils::fs to be more compliant with std::filesystem for an eventual C++17 migration and found the current functionality includes a path::exists() method which doesn't exist in std::filesystem. This simply changes usage to the non-member function and can be merged regardless of any future changes to rcpputils.

Signed-off-by: Josh Langsfeld <josh.langsfeld@gmail.com>
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Seems reasonable. I'll run CI on it next.

@clalancette
Copy link
Contributor

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

All right, CI looks good, so merging. Thanks for the contribution.

@clalancette clalancette merged commit b9ef1fe into ros2:master Jan 5, 2021
@jdlangs jdlangs deleted the fix/std-filesystem-compliance branch January 5, 2021 17:05
@jdlangs
Copy link
Contributor Author

jdlangs commented Jan 5, 2021

Thanks!

I'm not familiar with the procedure for getting stuff like this backported; what are the usual steps? Am I expected to take the lead on it?

@clalancette
Copy link
Contributor

I'm not familiar with the procedure for getting stuff like this backported; what are the usual steps? Am I expected to take the lead on it?

That's most expedient, yes. Though I'm not entirely sure this makes sense to backport. First, it is only in a test. And second, we won't be switching Foxy to C++17, so the fact that it isn't 100% compliant with std::filesystem doesn't matter as much. What's your thinking here?

@jdlangs
Copy link
Contributor Author

jdlangs commented Jan 6, 2021

It's really just so I don't have to keep around a forked version if combined with a modified rcpputils (since I'll have foxy versions as my baseline). But yeah, it's not a huge improvement since the std::filesystem stuff can't be backported anyways.

Is the process just opening a PR against foxy with this commit cherry-picked on to a new branch?

@clalancette
Copy link
Contributor

Is the process just opening a PR against foxy with this commit cherry-picked on to a new branch?

Yeah, that should do it for this PR.

@jdlangs
Copy link
Contributor Author

jdlangs commented Jan 6, 2021

Ok thanks; if it looks like it will save some effort I'll go ahead with that, otherwise I won't bother you guys.

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.

None yet

3 participants