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 boost filesystem exists #5907

Merged

Conversation

cybaol
Copy link
Contributor

@cybaol cybaol commented Dec 21, 2023

Task 3 of #5881

@cybaol cybaol closed this Dec 23, 2023
@cybaol cybaol deleted the replace_boost_filesystem_exists branch December 23, 2023 20:28
@cybaol cybaol restored the replace_boost_filesystem_exists branch December 24, 2023 12:37
@cybaol cybaol reopened this Dec 24, 2023
@mvieth
Copy link
Member

mvieth commented Dec 28, 2023

Hi, my idea was not to replace every boost::filesystem::exists with std::ifstream(filename).good(), but only where a std::ifstream is opened for reading anyways. For example in io/src/ifs_io.cpp: after the check with empty and exists, the file is opened anyways with a std::ifstream, so we could remove boost::filesystem::exists there and add !fs.good() after opening.
In other cases, where the file is not opened as a std::ifstream immediately afterwards, I would not try to replace boost::filesystem::exists with std::ifstream(filename).good(). Opening a filestream just to check whether a file exists seems kind of wasteful, and I would assume that Boost's exists is more elegant.
I have come to the conclusion that it does not seem possible to replace everything from boost::filesystem in PCL without using std::filesystem (C++17), at least not in an easy and elegant way. So I think it would be best to focus more on idea 3 from the issue (using both boost::filesystem and std::filesystem, and selecting the appropriate one with the preprocessor).

@mvieth
Copy link
Member

mvieth commented Dec 30, 2023

I believe it would make sense to first check whether the file_name is empty, before opening the filestream. That way we have made sure that the string contains something. We could also change the error message, for example "No file name given!" instead of "Could not find file ..." (or similar)

return (-1);
}

if (mtl_file_name.empty())
Copy link
Contributor Author

@cybaol cybaol Dec 31, 2023

Choose a reason for hiding this comment

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

Moved to pcl::MTLReader::read (const std::string& mtl_file_path).

Copy link
Member

Choose a reason for hiding this comment

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

But removing this check here would change the behaviour: if mtl_file_name is empty here, then mtl_file_path below would contain obj_file_path.parent_path() (and maybe a slash or backslash at the end), meaning mtl_file_path would not be empty. So as I see it, it is necessary to keep this check here.

Comment on lines 69 to 70
// Open file in binary mode to avoid problem of
// std::getline() corrupting the result of ifstream::tellg()
Copy link
Member

Choose a reason for hiding this comment

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

This comment does not make sense to me: std::getline is not used in ifs_io.cpp?

return (-1);
}

if (mtl_file_name.empty())
Copy link
Member

Choose a reason for hiding this comment

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

But removing this check here would change the behaviour: if mtl_file_name is empty here, then mtl_file_path below would contain obj_file_path.parent_path() (and maybe a slash or backslash at the end), meaning mtl_file_path would not be empty. So as I see it, it is necessary to keep this check here.

Copy link
Member

@mvieth mvieth left a comment

Choose a reason for hiding this comment

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

Thanks!

@mvieth mvieth merged commit 6e51b16 into PointCloudLibrary:master Jan 5, 2024
13 checks passed
@cybaol cybaol deleted the replace_boost_filesystem_exists branch January 5, 2024 14:09
mvieth added a commit to mvieth/pcl that referenced this pull request Apr 21, 2024
Partially revert PointCloudLibrary#5974 and PointCloudLibrary#5988 and PointCloudLibrary#5907
After the 1.14.1 release, we can re-apply these changes.
The ABI checker reported:
`The parameter file_name became passed in r12 register instead of r13. Applications will read the wrong memory block instead of the parameter value.` and `The parameter cloud_tgt_demean became passed in r12 register instead of r13.`
These changes are not super important, so I think it doesn't hurt to temporarily revert them to achieve 100% ABI compatibility between 1.14.0 and 1.14.1.
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.

2 participants