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

Fix function signature mismatching. #1625

Merged
merged 1 commit into from
Sep 1, 2016
Merged

Fix function signature mismatching. #1625

merged 1 commit into from
Sep 1, 2016

Conversation

renatoGarcia
Copy link
Contributor

Fix #1624

The function template pcl::io::save has a distinct signature on
declaration (pcl/io/auto_io.h) and definition (pcl/io/impl/auto_io.hpp).

On fixing that a bug popup on ifs_io.h. The conversion order between
PointCloud<> to/from PCLPointCloud2 was backward.

@@ -1365,6 +1366,36 @@ TEST (PCL, Locale)
#endif
}

//////////////////////////////////////////////////////////////////////////////////////////////////////////////////
TEST (PCL, AutoIO)
Copy link
Member

Choose a reason for hiding this comment

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

Since you're working with a PointCloud<PointT> type, i would recommend you write a typed test case like this but for all point types. The macro is PCL_POINT_TYPES.

@SergioRAgostinho SergioRAgostinho self-assigned this Jun 15, 2016
@SergioRAgostinho SergioRAgostinho added the needs: author reply Specify why not closed/merged yet label Jul 11, 2016
@SergioRAgostinho SergioRAgostinho added this to the pcl-1.8.1 milestone Jul 11, 2016
@renatoGarcia
Copy link
Contributor Author

I have implemented your suggestions, but there are errors arising when loading a .ply file for some point types. These errors are on functions that load the ply file, not on pcl::io::save() or pcl::io::load() that only route the call accordingly to a file extension.

For now I would suggest simply comment out the ply tests, as that doesn't invalidates nor is directly related to the bug being fixed by this PR.

@SergioRAgostinho SergioRAgostinho added needs: code review Specify why not closed/merged yet and removed needs: author reply Specify why not closed/merged yet labels Aug 18, 2016
@@ -1365,6 +1366,43 @@ TEST (PCL, Locale)
#endif
}

//////////////////////////////////////////////////////////////////////////////////////////////////////////////////
template <typename T> class AutoIOTest : public testing::Test { };
typedef ::testing::Types<BOOST_PP_SEQ_ENUM (PCL_POINT_TYPES)> PCLPointTypes;
Copy link
Member

@SergioRAgostinho SergioRAgostinho Aug 18, 2016

Choose a reason for hiding this comment

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

Change this to

typedef ::testing::Types<BOOST_PP_SEQ_ENUM (PCL_XYZ_POINT_TYPES PCL_NORMAL_POINT_TYPES)> PCLXyzNormalPointTypes;
TYPED_TEST_CASE (AutoIOTest, PCLXyzNormalPointTypes);

@SergioRAgostinho
Copy link
Member

Implement the change on the point types I just mentioned, uncomment the PLY tests, squash the commits and I think we're good to go.

@SergioRAgostinho SergioRAgostinho added needs: author reply Specify why not closed/merged yet and removed needs: code review Specify why not closed/merged yet labels Aug 18, 2016
@SergioRAgostinho
Copy link
Member

@jspricke Speaking of ABI breakage, what about this PR. We're changing the function signature, although it was wrong and not compiling so... Just want to be sure this should go on 1.8.1 :)

The function template pcl::io::save has a distinct signature on
declaration (pcl/io/auto_io.h) and definition (pcl/io/impl/auto_io.hpp).

On fixing that a bug popup on ifs_io.h. The conversion order between
PointCloud<> to/from PCLPointCloud2 was backward.
@renatoGarcia
Copy link
Contributor Author

renatoGarcia commented Aug 31, 2016

@SergioRAgostinho I have implemented the changes and squash.

@renatoGarcia
Copy link
Contributor Author

@SergioRAgostinho @jspricke As of API breakage, yes that could be a problem. Although in this function signature, as far as could search, the API has never worked to begin with. If I'm not wrong, the function was added in ab0ecd but only declaration, without a template function definition. The definition was added in a6f157, but already with the current argument mismatch.

@SergioRAgostinho SergioRAgostinho removed the needs: author reply Specify why not closed/merged yet label Aug 31, 2016
@SergioRAgostinho
Copy link
Member

Thanks! @VictorLamoine @taketwo opinions on this? 1.8.1 or 1.9.0?
My reasoning is it should still go to 1.8.1 because the original function could not be even be used/compiled, ergo we're not exactly breaking anything.

@VictorLamoine
Copy link
Contributor

1.8.1, I can't see how we could be breaking anyone's code with this PR.

@taketwo
Copy link
Member

taketwo commented Aug 31, 2016

Agree.

@SergioRAgostinho SergioRAgostinho merged commit 46cb8fe into PointCloudLibrary:master Sep 1, 2016
@renatoGarcia renatoGarcia deleted the auto_io_fix branch September 1, 2016 06:09
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.

pcl::io::save signature mismatching.
4 participants