Add pcl::make_shared that automatically handles aligned allocations

[aPonza]

Cherry-picked the relevant commits from #3146 to only add the function.

I would argue that this PR (plus the automated clang-tidy changes from the previous PR) eases the job of transitioning into C++14 and from boost::shared_ptr to std::shared_ptr in particular.

There is also left to discuss this comment from Sergio here (apart from the whole "do you want this").

[taketwo]

Hi, sorry for not joining the discussion earlier. Generally, 👍 for doing this. It's not a part of this PR and is left for discussion in future, but I can imagine eventually arriving at code like this, which I believe looks nice:

auto cloud_in = pcl::make_shared_cloud<pcl::PointXYZ>();

But back to the PR. Would it be too much if I ask you to regroup commits such that there are only two left:

• add pcl::make_shared, macro, and meta-functions
• find/replace EIGEN -> PCL

As it is now the find/replace produces a lot of noise and it's hard to concentrate on the meaningful changes.

Regarding point_traits -> type_traits: I support this, but we can do it separately.

[taketwo]

Thanks.

First, I wanted to propose a shorter implementation of has_custorm_allocator trait, based on void_t<> (which is a part of C++17):

template<typename...> using void_t = void; // part of std in c++17
template <typename, typename = void_t<>> struct has_custom_allocator : std::false_type { };
template <typename T> struct has_custom_allocator<T, void_t<typename T::custom_allocator>> : std::true_type { };

What's your opinion?

Second, I think we should add a simple unit test. I have thought about this, but don't have any good ideas how to verify that pcl::make_shared did the right thing. However, we can at least test that has_custom_allocator works.

Third, we'll need to add documentation to both make_shared and the macro. But let's think about this after we handle the first two points.

[SergioRAgostinho]

First, I wanted to propose a shorter implementation of has_custorm_allocator trait, based on void_t<> (which is a part of C++17):

template<typename...> using void_t = void; // part of std in c++17
template <typename, typename = void_t<>> struct has_custom_allocator : std::false_type { };
template <typename T> struct has_custom_allocator<T, void_t<typename T::custom_allocator>> 

If this mechanism is going to be available in C++17 then it is definitely best if we mimic it. Did you test if this particular implementation works? I'm asking because from quickly looking at it I'm seeing a potential ambiguous call error (which was what forced to adopt the other "sink function" mechanism) in case you pass to it a type to it which has a custom_allocator defined. Both these would be eligible for resolution.

template <typename, typename = void_t<>> struct has_custom_allocator : std::false_type { };
template <typename T> struct has_custom_allocator<T, void_t<typename T::custom_allocator>> : std::true_type { };

Second, I think we should add a simple unit test. I have thought about this, but don't have any good ideas how to verify that pcl::make_shared did the right thing. However, we can at least test that has_custom_allocator works.

Agreed. Supposedly, when using make shared, the control block and actual object being held are allocated in a single call. Ultimately, we only care if the held object is allocated on a multiple of X (16?) bits address. There's also the challenge of know what exactly is X. I believe this is configurable and platform dependent. We probably should look into Eigen's own unit tests for some guidance.

[aPonza]

I'm seeing a potential ambiguous call error (which was what forced to adopt the other "sink function" mechanism) in case you pass to it a type to it which has a custom_allocator defined. Both these would be eligible for resolution.

This answer explains it thoroughly, but basically if Foo::custom_allocator(_type?) is well-formed there's no Substitution Failure and the specialized non-primary definition gets chosen unequivocally.

[SergioRAgostinho]

I was convinced I had tried something along these lines before, but I clearly failed some of the underlying constructs. Nevertheless, this new proposal is substantially more elegant than mine and hopefully, it will allows us to perform a quick find replace to remove the custom version once we adopt C++17, so I'm completely in favor.

Edit: @aPonza - rebase on top of master instead of merging master to your own branch. You will still need to go through the same merge conflicts, but you won't have the extra merge commits at the end.

[aPonza]

Not sure why it shows the same merge commit twice, but I'll be sure to rebase next time... Anyways, I don't understand what I'm missing: why is this not working?

#include <gtest/gtest.h>
#include <Eigen/Core>
#include <boost/make_shared.hpp>
#include <boost/shared_ptr.hpp>
#include <pcl/pcl_macros.h>

struct Foo
{
public:
  PCL_MAKE_ALIGNED_OPERATOR_NEW
};

boost::shared_ptr<Foo> make1() { return boost::shared_ptr<Foo>(new Foo); } // I'd expect no error
Foo* make2() { return new Foo; } // I'd expect no error
boost::shared_ptr<Foo> make3() { return boost::allocate_shared<Foo>(Eigen::aligned_allocator<Foo>()); } // I'd expect no error
boost::shared_ptr<Foo> make4() { return boost::make_shared<Foo>(); }  // I'd expect an error

TEST (MakeShared, HasCustomAllocator)
{
  auto a = make1();
  EXPECT_EQ(sizeof(a), 16u);

  auto b = make2();
  EXPECT_EQ(sizeof(b), 16u);

  auto c = make3();
  EXPECT_EQ(sizeof(c), 16u);

  auto d = make4();
  EXPECT_EQ(sizeof(d), 16u);
}

which outputs

$ test/common/test_make_shared 
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from MakeShared
[ RUN      ] MakeShared.HasCustomAllocator
<...>/pcl/test/common/test_make_shared.cpp:61: Failure
Value of: 16u
  Actual: 16
Expected: sizeof(b)
Which is: 8
[  FAILED  ] MakeShared.HasCustomAllocator (0 ms)
[----------] 1 test from MakeShared (0 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (1 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] MakeShared.HasCustomAllocator

 1 FAILED TEST

b shouldn't give problems though...

My knowledge in unit/integration testing is nil, I tried to look up examples in eigen like you suggested but couldn't yet find the relevant ones. I have found the gcc tests though, I suppose the difference between allocate_shared and make_shared is what you're after.

[taketwo]

I'd avoid getting deep into this. Eigen's own tests of aligned allocators are complex and hard to understand. I think we should just assume they work. On our side, we only need to check whether our dispatching between aligned and normal make_shared is correct. And since dispatching is based on the trait, we should just check the trait.

[SergioRAgostinho]

boost::shared_ptr<Foo> make4() { return boost::make_shared<Foo>(); }  // I'd expect an error

There should exists no error in this situation. You're simply allocating a non-aligned Foo object. On its own this won't cause any errors. Now if you have a fixed size Eigen attribute inside e.g., Eigen::Vector4f and you try to apply some math operation on it, then you will get a segfault.

b shouldn't give problems though...

b is a simple pointer. It's is gonna be word sized which for your machine and OS is 8 bytes.

[aPonza]

No clue what I did wrong, after I saw the mess I ended up doing it the other way, I'll read up on it.

Regardless, I don't understand if this is what you were expecting for the check, it seems simple, if anything. I couldn't understand how to use googlemock's EXPECT_CALL to check for pcl::make_shareds dispatch to boost::make_shared or boost::allocate_shared. The file is named like this in case you eventually want to check for death or other related tests.

EDIT: took a while but I fixed it.

[aPonza]

Both failures are from #3194 as far as I understand.

[taketwo]

Thanks. Besides from a single comment that I left this PR looks complete in terms of code. Before we merge it would be nice to add better docstrings though. Since we are dealing with macros and "overloaded" template functions, we'd need to double-check empirically that Doxygen generates appropriate HTML. I'll try to have a look at this tomorrow and get back with details on how to format docstrings.

[taketwo]

I figured out how add documentation to make_shared. We'll need a conditional "documentation" branch with documentation and SFINAE-free signature of the function, and the "implementation" branch with actual implementation:

#ifdef DOXYGEN_ONLY

/// Documentation
template<typename T, typename ... Args>
boost::shared_ptr<T> make_shared(Args&&... args);

#else

template<typename T, typename ... Args>
std::enable_if_t<has_custom_allocator<T>::value, boost::shared_ptr<T>> make_shared(Args&&... args)
{
  return boost::allocate_shared<T>(Eigen::aligned_allocator<T>(), std::forward<Args> (args)...);
}

template<typename T, typename ... Args>
std::enable_if_t<!has_custom_allocator<T>::value, boost::shared_ptr<T>> make_shared(Args&&... args)
{
  return boost::make_shared<T>(std::forward<Args> (args)...);
}

#endif

To make this work we need to add DOXYGEN_ONLY to this list in Doxygen config.

Now as for the macro documentation, I'm struggling to make it appear in HTML. If someone has experience with this, please let me know.

[taketwo]

Thanks. Apart from the question with the name, LGTM. @SergioRAgostinho do you have suggestions?

[taketwo]

By the way, #3194 was solved, please rebase so that we can see Windows status as well.

[SergioRAgostinho]

LGTM. I just have minor comments.

[SergioRAgostinho]

Keyboard gone wild?

[SergioRAgostinho]

The colon is still here. You mark this as resolved. Was it intentional or is this a bug of my browser cache?

[taketwo]

You are right, it's still there.

[taketwo]

Let's regroup commits before merging. I propose to have three:

1. Add make_shared and trait
2. Add unit test
3. Replace EIGEN_MAKE... with PCL_MAKE...

1 and 2 may be joined if you prefer.

[SergioRAgostinho]

The colon is still here. You mark this as resolved. Was it intentional or is this a bug of my browser cache?

[aPonza]

Absolutely unintentional, something went wrong while squashing the commits, I'll recheck and fix.

EDIT: it really is weird, it's still in the reflog and I can see I had it, and it's the only thing missing... there is the fix in a4c809b and it's wrongly reset in e485ca0. No clue.