Change PCL smart pointers from `boost` to `std` by aPonza · Pull Request #3750 · PointCloudLibrary/pcl

aPonza · 2020-03-14T18:46:14Z

Hoping to close #3735

Also closes #2792

aPonza · 2020-03-14T20:26:24Z

Windows error (link):

C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\type_traits(1271): error C2338: You've instantiated std::aligned_storage<Len, Align> with an extended alignment (in other words, Align > alignof(max_align_t)). Before VS 2017 15.8, the member type would non-conformingly have an alignment of only alignof(max_align_t). VS 2017 15.8 was fixed to handle this correctly, but the fix inherently changes layout and breaks binary compatibility (*only* for uses of aligned_storage with extended alignments). Please define either (1) _ENABLE_EXTENDED_ALIGNED_STORAGE to acknowledge that you understand this message and that you actually want a type with an extended alignment, or (2) _DISABLE_EXTENDED_ALIGNED_STORAGE to silence this message and get the old non-conformant behavior. (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp) [D:\a\build\filters\pcl_filters.vcxproj]
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\type_traits(1291): note: see reference to class template instantiation 'std::_Aligned<112,16,double,false>' being compiled (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\type_traits(1298): note: see reference to class template instantiation 'std::_Aligned<112,16,int,false>' being compiled (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\type_traits(1305): note: see reference to class template instantiation 'std::_Aligned<112,16,short,false>' being compiled (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\type_traits(1312): note: see reference to class template instantiation 'std::_Aligned<112,16,char,false>' being compiled (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\type_traits(1320): note: see reference to class template instantiation 'std::aligned_storage<112,16>' being compiled (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\type_traits(1352): note: see reference to alias template instantiation 'std::aligned_storage_t<112,16>' being compiled (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\type_traits(1357): note: see reference to class template instantiation 'std::aligned_union<1,_Ty>' being compiled
          with
          [
              _Ty=pcl::PointCloud<pcl::PointXYZ>
          ] (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\memory(1844): note: see reference to alias template instantiation 'std::aligned_union_t<1,pcl::PointCloud<PointT>>' being compiled
          with
          [
              PointT=pcl::PointXYZ
          ] (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\memory(1887): note: see reference to class template instantiation 'std::_Ref_count_obj_alloc<_Ty,_Alloc>' being compiled
          with
          [
              _Ty=pcl::PointCloud<pcl::PointXYZ>,
              _Alloc=Eigen::aligned_allocator<pcl::PointCloud<pcl::PointXYZ>>
          ] (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  D:\a\1\s\common\include\pcl/make_shared.h(74): note: see reference to function template instantiation 'std::shared_ptr<pcl::PointCloud<PointT>> std::allocate_shared<T,Eigen::aligned_allocator<T>,>(const _Alloc &)' being compiled
          with
          [
              PointT=pcl::PointXYZ,
              T=pcl::PointCloud<pcl::PointXYZ>,
              _Alloc=Eigen::aligned_allocator<pcl::PointCloud<pcl::PointXYZ>>
          ] (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  D:\a\1\s\filters\src\radius_outlier_removal.cpp(167): note: see reference to function template instantiation 'std::shared_ptr<pcl::PointCloud<PointT>> pcl::make_shared<pcl::PointCloud<PointT>,>(void)' being compiled
          with
          [
              PointT=pcl::PointXYZ
          ]
  normal_space.cpp

aPonza · 2020-03-14T21:03:41Z

I might be completely off, but from what I gather from here it's not so much an issue coming from the switch, but more like coming from a missing PCL_MAKE_ALIGNED_OPERATOR_NEW in some aligned class the code is instantiating with a make_shared instead of an allocate_shared.

There is only one pcl::make_shared instance in the referenced code

pcl/filters/src/radius_outlier_removal.cpp

Line 167 in 20a289b

auto cloud = pcl::make_shared<pcl::PointCloud<pcl::PointXYZ>> ();

but I can't find eigen members in the neighborhood, so I might be completely off.

If that's the case, their suggestion is pretty straightforward though: add

if(MSVC AND ${MSVC_VERSION} GREATER_EQUAL 1915)
  # You must acknowledge that you understand MSVC resolved a byte alignment issue in this compiler
  # We get this due to using Eigen objects and allocating those objects with make_shared
target_compile_definitions( ${target_name} PRIVATE _ENABLE_EXTENDED_ALIGNED_STORAGE )
endif()

at line 127

pcl/filters/CMakeLists.txt

Lines 125 to 130 in 20a289b

    
           set(LIB_NAME "pcl_${SUBSYS_NAME}") 
        
           include_directories("${CMAKE_CURRENT_SOURCE_DIR}/include") 
        
           PCL_ADD_LIBRARY(${LIB_NAME} COMPONENT ${SUBSYS_NAME} SOURCES ${srcs} ${incs} ${impl_incs}) 
        
           target_link_libraries("${LIB_NAME}" pcl_common pcl_sample_consensus pcl_search pcl_kdtree pcl_octree) 
        
           PCL_MAKE_PKGCONFIG(${LIB_NAME} COMPONENT ${SUBSYS_NAME} DESC ${SUBSYS_DESC} PCL_DEPS ${SUBSYS_DEPS})

Thoughts?

aPonza · 2020-03-15T00:35:27Z

You can see I edited the windows error message because I realized it's a much longer error. It is of course picking up on the std::allocate_shared. I don't understand why Windows should only complain here, though...anyways I'll push the suggested fix as soon as I know it's the only failing job.

kunaltyagi

Maybe split this PR in 2 parts. One for the shift from boost::shared_ptr to std::shared_ptr and another for boost::->pcl::. The second one can be merged faster, maybe in time for 1.10.1 and the first one afterwards.

apps/src/organized_segmentation_demo.cpp

registration/include/pcl/registration/correspondence_rejection_distance.h

aPonza · 2020-03-15T10:28:11Z

There is also a single boost::weak_ptr -> std::weak_ptr to decide upon. I can see the whole codebase has 3 weak ptrs though, and two of them are already std somehow.

I'm rebasing and regrouping the commits to ease my work afterwards.

SergioRAgostinho · 2020-03-15T10:32:14Z

There is also a single boost::weak_ptr -> std::weak_ptr to decide upon. I can see the whole codebase has 3 weak ptrs though, and two of them are already std somehow.

They probably were not a part of the public interface and got swapped :)

aPonza · 2020-03-15T10:44:02Z

They are not, but neither is this one. Separate PR, I guess.

kunaltyagi · 2020-03-15T11:42:04Z

Using the old definition of the label needs: pr merge

aPonza · 2020-03-15T14:26:29Z

neither is this one

This is false. It's part of the public API because it's locked and returned

pcl/io/src/openni_camera/openni_driver.cpp

Lines 254 to 291 in e6e7377

    
           auto device = device_context_[index].device.lock (); 
        
           if (!device) 
        
           { 
        
             unsigned short vendor_id; 
        
             unsigned short product_id; 
        
             getDeviceType (device_context_[index].device_node.GetCreationInfo (), vendor_id, product_id ); 
        
             if (vendor_id == 0x45e) 
        
             { 
        
               device.reset (new DeviceKinect (context_,  
        
                                               device_context_[index].device_node, 
        
                                               *device_context_[index].image_node,  
        
                                               *device_context_[index].depth_node, 
        
                                               *device_context_[index].ir_node)); 
        
               device_context_[index].device = device; 
        
             } 
        
             else if (vendor_id == 0x1d27) 
        
             { 
        
               if (device_context_[index].image_node.get()) 
        
                 device.reset (new DevicePrimesense (context_,  
        
                                                     device_context_[index].device_node,  
        
                                                     *device_context_[index].image_node,  
        
                                                     *device_context_[index].depth_node, 
        
                                                     *device_context_[index].ir_node)); 
        
               else 
        
                 device.reset (new DeviceXtionPro (context_,  
        
                                                   device_context_[index].device_node,  
        
                                                   *device_context_[index].depth_node,  
        
                                                   *device_context_[index].ir_node)); 
        
               device_context_[index].device = device; 
        
             } 
        
             else 
        
             { 
        
               THROW_OPENNI_EXCEPTION ("Vendor %s (%s) unknown!", 
        
                                       getVendorName (index), vendor_id); 
        
             } 
        
           } 
        
           return (device);

taketwo · 2020-03-20T16:56:05Z

What's the status of this one?

aPonza · 2020-03-20T17:40:34Z

Last night I was working a bit on the weak_ptr PR, this one should follow suit after that one.

aPonza · 2020-03-20T18:09:58Z

Also I'm still waiting on static/dynamic_pointer_cast news.

SergioRAgostinho · 2020-03-21T09:39:58Z

Sorry for that. My opinion is to try and go unqualified.

aPonza · 2020-03-21T22:12:50Z

Ouch! So the flag isn't correctly propagated AND it was completely fine with boost pointers!

C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\type_traits(1271): error C2338: You've instantiated std::aligned_storage<Len, Align> with an extended alignment (in other words, Align > alignof(max_align_t)). Before VS 2017 15.8, the member type would non-conformingly have an alignment of only alignof(max_align_t). VS 2017 15.8 was fixed to handle this correctly, but the fix inherently changes layout and breaks binary compatibility (*only* for uses of aligned_storage with extended alignments). Please define either (1) _ENABLE_EXTENDED_ALIGNED_STORAGE to acknowledge that you understand this message and that you actually want a type with an extended alignment, or (2) _DISABLE_EXTENDED_ALIGNED_STORAGE to silence this message and get the old non-conformant behavior. (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp) [D:\a\build\filters\pcl_filters.vcxproj]
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\type_traits(1291): note: see reference to class template instantiation 'std::_Aligned<112,16,double,false>' being compiled (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\type_traits(1298): note: see reference to class template instantiation 'std::_Aligned<112,16,int,false>' being compiled (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\type_traits(1305): note: see reference to class template instantiation 'std::_Aligned<112,16,short,false>' being compiled (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\type_traits(1312): note: see reference to class template instantiation 'std::_Aligned<112,16,char,false>' being compiled (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\type_traits(1320): note: see reference to class template instantiation 'std::aligned_storage<112,16>' being compiled (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\type_traits(1352): note: see reference to alias template instantiation 'std::aligned_storage_t<112,16>' being compiled (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\type_traits(1357): note: see reference to class template instantiation 'std::aligned_union<1,_Ty>' being compiled
          with
          [
              _Ty=pcl::PointCloud<pcl::PointXYZ>
          ] (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\memory(1844): note: see reference to alias template instantiation 'std::aligned_union_t<1,pcl::PointCloud<PointT>>' being compiled
          with
          [
              PointT=pcl::PointXYZ
          ] (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\VC\Tools\MSVC\14.16.27023\include\memory(1887): note: see reference to class template instantiation 'std::_Ref_count_obj_alloc<_Ty,_Alloc>' being compiled
          with
          [
              _Ty=pcl::PointCloud<pcl::PointXYZ>,
              _Alloc=Eigen::aligned_allocator<pcl::PointCloud<pcl::PointXYZ>>
          ] (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  D:\a\1\s\common\include\pcl/memory.h(127): note: see reference to function template instantiation 'std::shared_ptr<pcl::PointCloud<PointT>> std::allocate_shared<T,Eigen::aligned_allocator<T>,>(const _Alloc &)' being compiled
          with
          [
              PointT=pcl::PointXYZ,
              T=pcl::PointCloud<pcl::PointXYZ>,
              _Alloc=Eigen::aligned_allocator<pcl::PointCloud<pcl::PointXYZ>>
          ] (compiling source file D:\a\1\s\filters\src\radius_outlier_removal.cpp)
  D:\a\1\s\filters\src\radius_outlier_removal.cpp(167): note: see reference to function template instantiation 'std::shared_ptr<pcl::PointCloud<PointT>> pcl::make_shared<pcl::PointCloud<PointT>,>(void)' being compiled
          with
          [
              PointT=pcl::PointXYZ
          ]
  random_sample.cpp
  normal_space.cpp

kunaltyagi · 2020-03-25T19:06:22Z

Maintainer note: needs rebase post #3770 merge.

aPonza · 2020-03-25T21:24:13Z

And also post #3753. We have a test failure on Windows x86:

  109: [----------] 1 test from SacTest/1, where TypeParam = class pcl::LeastMedianSquares<struct pcl::PointXYZ>
  109: [ RUN      ] SacTest/1.InfiniteLoop
  109: D:\a\1\s\test\sample_consensus\test_sample_consensus.cpp(134): error: Expected equality of these values:
  109:   std::cv_status::no_timeout
  109:     Which is: 4-byte object <00-00 00-00>
  109:   cv.wait_for (lock, 1s)
  109:     Which is: 4-byte object <01-00 00-00>
  109: [  FAILED  ] SacTest/1.InfiniteLoop, where TypeParam = class pcl::LeastMedianSquares<struct pcl::PointXYZ> (1001 ms)
  109: [----------] 1 test from SacTest/1 (1001 ms total)

kunaltyagi · 2020-03-26T04:48:55Z

Flaky test. Rerunning it

kunaltyagi · 2020-04-09T12:40:11Z

Some changes seem to be duplicated across the PRs

aPonza · 2020-04-09T12:42:00Z

This one checks if everything works after the whole transition is complete, the extracted PRs check the single changes? I can cancel the build if CI time is precious. I'll remove the commits as soon as the other PRs are merged.

EDIT: and this PR has the smaller weak_ptr change alternative to the larger one in 3753

kunaltyagi · 2020-04-09T12:44:43Z

No need to cancel CI. I meant that some parts of the changeset were duplicated in other PR too, like editing the include files, etc. Maybe add a preferred sequence to the PRs submitted, so we can prioritize the ones that need to go first

aPonza · 2020-04-09T12:51:35Z

Well this is the last one, it requires #3753 (maybe?), #3770, and #3893 in not any particular order.

aPonza · 2020-04-19T15:04:49Z

This won't compile until #3753 is merged. Should be the last remaining dependency PR-wise.

EDIT: it'll also show unrelated changes until a rebase after that PR is merged

kunaltyagi · 2020-05-03T06:51:17Z

Time to rebase and test 😄

…mmon

kunaltyagi

LGTM

SergioRAgostinho · 2020-05-03T15:19:38Z

Flagging it an API break. Just because we provided a pcl::shared_ptr it doesn't mean people could not pass directly boost::shared_ptrs to the interfaces. Hence we break the api with this change.

Good to merge from my side once CI is done.

kunaltyagi added this to the pcl-1.11.0 milestone Mar 15, 2020

kunaltyagi added the changelog: ABI break Meta-information for changelog generation label Mar 15, 2020

kunaltyagi reviewed Mar 15, 2020

View reviewed changes

apps/src/organized_segmentation_demo.cpp Outdated Show resolved Hide resolved

registration/include/pcl/registration/correspondence_rejection_distance.h Outdated Show resolved Hide resolved

aPonza marked this pull request as ready for review March 15, 2020 10:14

aPonza force-pushed the shared_ptrs branch from 376f5b7 to be45089 Compare March 15, 2020 10:47

This was referenced Mar 15, 2020

Converting internal use of boost::make_shared to pcl::make_shared #3752

Merged

Add pcl::weak_ptr to have a single-switch move from boost:: to std:: weak pointers #3753

Merged

kunaltyagi added the needs: pr merge Specify why not closed/merged yet label Mar 15, 2020

aPonza force-pushed the shared_ptrs branch from be45089 to 0faeaa2 Compare March 20, 2020 18:08

aPonza mentioned this pull request Mar 21, 2020

Provide dynamic and static pointer casts in pcl namespace to allow easy migration from boost to std smart pointers #3770

Merged

aPonza mentioned this pull request Apr 8, 2020

Remove boost shared pointer usage and headers in favor of pcl/memory.h #3893

Merged

aPonza force-pushed the shared_ptrs branch 3 times, most recently from 207eab8 to 540f673 Compare April 9, 2020 12:18

aPonza force-pushed the shared_ptrs branch from 540f673 to 5d2bf40 Compare April 9, 2020 12:24

aPonza force-pushed the shared_ptrs branch from 5d2bf40 to 77a249c Compare April 19, 2020 15:04

kunaltyagi added needs: more work Specify why not closed/merged yet changelog: new feature Meta-information for changelog generation and removed needs: pr merge Specify why not closed/merged yet labels May 3, 2020

kunaltyagi changed the title ~~From boost:: to std:: shared pointers~~ Change PCL smart pointers from boost to std May 3, 2020

Andrea Ponza added 2 commits May 3, 2020 14:34

switch from boost:: to std:: in memory.h

11f5cd0

propagate _ENABLE_EXTENDED_ALIGNED_STORAGE to modules linking with co…

e90d40e

…mmon

aPonza force-pushed the shared_ptrs branch from 77a249c to e90d40e Compare May 3, 2020 12:35

kunaltyagi added needs: code review Specify why not closed/merged yet and removed needs: more work Specify why not closed/merged yet labels May 3, 2020

kunaltyagi requested a review from SergioRAgostinho May 3, 2020 12:52

kunaltyagi approved these changes May 3, 2020

View reviewed changes

SergioRAgostinho added changelog: API break Meta-information for changelog generation and removed changelog: ABI break Meta-information for changelog generation needs: code review Specify why not closed/merged yet labels May 3, 2020

SergioRAgostinho approved these changes May 3, 2020

View reviewed changes

kunaltyagi merged commit 917c22c into PointCloudLibrary:master May 3, 2020

This was referenced May 4, 2020

[compile error] test_outofcore doesn't compile #4033

Closed

Fix compile issue in OutofcoreOctreeBase::setLODFilter after switching from boost::shared_ptr to std::shared_ptr #4081

Merged

niwhsa9 mentioned this pull request Oct 24, 2022

False positive detection on ArUco tags + PCL umrover/mrover-ros#189

Merged

Uh oh!

Conversation

aPonza commented Mar 14, 2020 • edited by kunaltyagi Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aPonza commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aPonza commented Mar 14, 2020

Uh oh!

aPonza commented Mar 15, 2020

Uh oh!

kunaltyagi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aPonza commented Mar 15, 2020

Uh oh!

SergioRAgostinho commented Mar 15, 2020

Uh oh!

aPonza commented Mar 15, 2020

Uh oh!

kunaltyagi commented Mar 15, 2020

Uh oh!

aPonza commented Mar 15, 2020

Uh oh!

taketwo commented Mar 20, 2020

Uh oh!

aPonza commented Mar 20, 2020

Uh oh!

aPonza commented Mar 20, 2020

Uh oh!

SergioRAgostinho commented Mar 21, 2020

Uh oh!

aPonza commented Mar 21, 2020

Uh oh!

kunaltyagi commented Mar 25, 2020

Uh oh!

aPonza commented Mar 25, 2020

Uh oh!

kunaltyagi commented Mar 26, 2020

Uh oh!

kunaltyagi commented Apr 9, 2020

Uh oh!

aPonza commented Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kunaltyagi commented Apr 9, 2020

Uh oh!

aPonza commented Apr 9, 2020

Uh oh!

aPonza commented Apr 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kunaltyagi commented May 3, 2020

Uh oh!

kunaltyagi left a comment

Choose a reason for hiding this comment

Uh oh!

SergioRAgostinho commented May 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aPonza commented Mar 14, 2020 •

edited by kunaltyagi

Loading

aPonza commented Mar 14, 2020 •

edited

Loading

aPonza commented Apr 9, 2020 •

edited

Loading

aPonza commented Apr 19, 2020 •

edited

Loading