Add emplace[_back] to pcl::PointCloud

[Morwenn]

Here is the pull request to implement #3204. It's my first time contributing to PCL, so I opened this pull request quite soon to ask questions about contribution. First of all, I haven't run the tests because I'm not sure how to run the test suite just yet, I merely know that it uses Google Test, and I suppose that there is an option in the CMakeLists.txt to run the test (consider it untested code pushed for the sake of the pull request). Anyway, here are my more specific questions & remarks:

• I couldn't find any variadic templates in PCL so far, is the formatting ok or would you like the spaces in other places.
• Is the documentation enough?
• Since C++17, std::vector::emplace_back returns a reference to the inserted element, but since the next version of PCL targets C++14, I couldn't really implement that short of querying the emplaced element before returning it. Is it okay or do you want the function to return a reference?
• I hesitated to #include <utility> but it seems that the whole file doesn't care much about include standard or Boost headers, so I decided to leave it out.
• Have I missed anything?

That's pretty much it. My IDE automatically removes the blank characters at the end of lines, I hope that it isn't too much noise.

[Morwenn]

It looks like the tests seredenpitously passed, which is nice I guess.

[SergioRAgostinho]

Looks good to me.

First of all, I haven't run the tests because I'm not sure how to run the test suite just yet, I merely know that it uses Google Test, and I suppose that there is an option in the CMakeLists.txt to run the test (consider it untested code pushed for the sake of the pull request)

Pass -DBUILD_global_tests=ON during configuration. If googletest is not installed in some visible path (often the case) you will need to also pass -DGTEST_INCLUDE_DIR=<prefix>/googletest/include -DGTEST_SRC_DIR=<prefix>/googletest. One of these is redundant but we never got around to patch it. Then run make tests.

• I couldn't find any variadic templates in PCL so far, is the formatting ok or would you like the spaces in other places.

It looked ok to me. Clang format will be integrated soon, so there's no need to worry much about it.

• Since C++17, std::vector::emplace_back returns a reference to the inserted element, but since the next version of PCL targets C++14, I couldn't really implement that short of querying the emplaced element before returning it. Is it okay or do you want the function to return a reference?

Given that I would say we would return the reference as well. It won't hurt C++14 users and by the time we need to adopt C++17, we don't have to do anything.

• I hesitated to #include <utility> but it seems that the whole file doesn't care much about include standard or Boost headers, so I decided to leave it out.

I'm failing to see the rationale here. Why would you want to explicitly include it?

Edit: I forgot to add. The unit tests feel very incomplete, but I also see that you simply followed what was done before. I'm not sure if we want to address them properly on this PR, just for emplace and emplace_back.

[Morwenn]

I'm failing to see the rationale here. Why would you want to explicitly include it?

I just tend to explicitly include every header I use in my own projects to avoid relying on transitive includes and witnessing some code break when the headers I'm using are not transitively included anymore (which happened from time to time, and broke differently depending on how the standard libraries transitively include standard headers themselves).

[taketwo]

👍 for including utility since this is where std::forward lives. Including dependencies explicitly is a good thing and we should move in that direction.

👍 for returning a reference for the reasons Sergio gave.

[Morwenn]

Ok, I'll change it tonight so that it returns a reference.

Concerning the unit tests, I just somewhat completed the existing ones, but it's otherwise mostly forwarding parameters to the corresponding std::vector functions and copying the few corresponding lines from push_back and insert, so the chances to get it wrong seemed quite low. I did hesitate to replace some instances of push_back in the test suite where emplace_back would have been more suitable.

[SergioRAgostinho]

I did hesitate to replace some instances of push_back in the test suite where emplace_back would have been more suitable.

I would suggest to leave that to another PR. In general these tend to create a lot of changes, which might require discussions of their own, delaying the merging of the new functionality. I rather have the functionality merged first and then we proceed with the appropriate replacements.