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

Replace dirent with boost::filesystem #3676

Merged
merged 4 commits into from
Feb 27, 2020

Conversation

shrijitsingh99
Copy link
Contributor

As suggested in #2888, replaced dirent with boost::filesystem

@kunaltyagi
Copy link
Member

With Boost, there's no need of keeping it exclusive to only non-Windows users

@kunaltyagi kunaltyagi added the status: triage Labels incomplete label Feb 25, 2020
@shrijitsingh99
Copy link
Contributor Author

shrijitsingh99 commented Feb 25, 2020

With Boost, there's no need of keeping it exclusive to only non-Windows users

Done
None of these functions in file_io.hpp are covered by any of the tests. Is a test needed for them?

common/include/pcl/common/impl/file_io.hpp Outdated Show resolved Hide resolved
common/include/pcl/common/impl/file_io.hpp Outdated Show resolved Hide resolved
common/include/pcl/common/impl/file_io.hpp Outdated Show resolved Hide resolved
@kunaltyagi kunaltyagi added changelog: enhancement Meta-information for changelog generation module: common platform: linux platform: windows and removed status: triage Labels incomplete labels Feb 25, 2020
@taketwo
Copy link
Member

taketwo commented Feb 26, 2020

You know, I'm skeptical about this entire file common/file_io.h. It provides tiny subset of boost::filesystem functionality. Do we need it? Both PCL and downstream projects should just use boost::filesystem directly.

@kunaltyagi
Copy link
Member

It provides tiny subset of boost::filesystem functionality

This wouldn't be an issue if the interface was a bit better. I mean it's a bit of annoyance to iterate over a directory and check each file. Much better to have a nice interface which acts like a generic filter

for (const auto file: pcl::io::with_extension(pcl::io::get_readable_files("directory"), ".pcd"))
...

std::vector<std::string> files;
pcl::io::getAllPcdFilesInDirectory("directory", files);
for (const auto file: files)
...

Without these 10 lines, each project will have to reproduce this. And it has pitfalls, like extension() having a dot, file iterator giving broken symlinks, etc. which we can remove

common/include/pcl/common/impl/file_io.hpp Outdated Show resolved Hide resolved
common/include/pcl/common/impl/file_io.hpp Outdated Show resolved Hide resolved
common/include/pcl/common/file_io.h Outdated Show resolved Hide resolved
@taketwo
Copy link
Member

taketwo commented Feb 27, 2020

Much better to have a nice interface which acts like a generic filter

OK, I agree that in principle we can provide a nicer interface on top of boost::filesystem. (But this is not what is available in file_io.h at teh moment 😆 )

@taketwo taketwo merged commit 6a51bbb into PointCloudLibrary:master Feb 27, 2020
@shrijitsingh99 shrijitsingh99 deleted the boost-filesystem branch February 27, 2020 12:10
@shrijitsingh99 shrijitsingh99 restored the boost-filesystem branch February 27, 2020 12:10
@shrijitsingh99 shrijitsingh99 deleted the boost-filesystem branch March 12, 2020 20:52
@taketwo taketwo changed the title Replace dirent with boost::filesystem Replace dirent with boost::filesystem Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants