Deprecate pcl/make_shared.h header; extract PCL_MAKE_ALIGNED_OPERATOR_NEW into pcl/memory.h

[aPonza]

This consolidates all memory-related functions, typedefs, and macros in a new header file pcl/memory.h. The old pcl/make_shared.h is deprecated.

Solves an issue in #3545.

[kunaltyagi]

PTAL at my change a4e46e7 where I've moved around a different (and smaller) set of includes.

[aPonza]

@kunaltyagi so you're saying std -> library -> 3rd party instead of library -> std -> 3rd party? I remember arguments in favor of both, and I see different approaches in PCL's files' convention (if there is one).

[taketwo]

I had the impression that we want to have "library -> 3rd party -> std", but this was not codified yet.

[kunaltyagi]

The order is not the concern. The files used is one. I move Eigen/Core and you move a lot more (indirectly included via pcl_macro).

As for the order, both have pro and cons. I don't really mind changing the order. The reason I prefer std first is because I trust the files on top to not include/declare anything that might cause a problem for later includes. So my personal order is std->3rd_party->library in a decreasing order of trust.

lib first approach allows for dependency check at a glance which is also nice.

[SergioRAgostinho]

I had the impression that we want to have "library -> 3rd party -> std", but this was not codified yet.

This. To catch issues with potential transitive dependencies early on.

[aPonza]

I tried, it works here, let me see what CI says.

EDIT: there was an extra rename for pcl_macros.h -> eigen_base.h in point_types.hpp

[aPonza]

Formatting is locally fixed (includes in alphabetic order), tutorials are locally fixed (forgot to add the new header in CMakeLists.txt), but Build.Ubuntu-16.04 fails for a PCL_EXPORTS error:

[ 16%] Building CXX object simulation/CMakeFiles/pcl_simulation.dir/src/camera.cpp.o
In file included from /__w/1/s/simulation/src/camera.cpp:2:0:
/__w/1/s/simulation/include/pcl/simulation/camera.h:12:19: error: variable 'pcl::simulation::PCL_EXPORTS pcl::simulation::Camera' has initializer but incomplete type
 class PCL_EXPORTS Camera {
                   ^

which I don't understand. The error is for forward declarations which require full types, but pcl_exports.h is about NVCC. I'm thinking eigen_base.h is lacking a way to avoid NVCC like pcl_macros.h used to due to Eigen/Core, but don't know what or where yet.

EDIT: it's a transitive dependency issue on PCL_EXPORTS, let me retest

[kunaltyagi]

We can remove pcl_exports now that pcl_macros is about to be nvcc compatible (Not now now; soon: post deprecated PR merge #3634) with the removal of Eigen/Core.

As for the error, it seems like PCL_EXPORTS is not found (since the compiler assumes it is in simulation namespace). You might need to include pcl_macros wherever the relevant macros are used.

[aPonza]

I can build pcl_simulation, there was also a missing couple of boost:: on top of the missing header.

[taketwo]

I'm not sure if global replace of pcl_macros.h with eigen_base.h is reasonable. That file provided a bit more than PCL_MAKE_ALIGNED_ALLOCATOR_NEW and we need to check at every include site what actually was used.

Also, sorry for the delay, I'd like to do a bit of name bikeshedding. What do you think about eigen.h instead of eigen_base.h? It's more common throughout PCL to have headers named after third-party libraries, without suffixes.

[kunaltyagi]

What do you think about eigen.h instead of eigen_base.h

Not opposed. But it'd require renaming the existing eigen.h to something else like eigen_operations.h or similar

I'm not sure if global replace of pcl_macros.h with eigen_base.h is reasonable.

It's not. At the very minimum all files without pcl_macros but with eigen_base and PCL_EXPORT will need pcl_macros.

[taketwo]

I have a feeling we are going to make some hastily moves now for no good reason and break something. Let's limit this PR to removing undefs and fixing the order of includes in VTK header. Reorganization of pcl_macros.h, pcl_exports.h, point_traits.h, etc is a bigger topic that has to be handled with care.

[aPonza]

What do you think about eigen.h instead of eigen_base.h?

As Kunal points out (and I did too, here) there are already files named eigen.h in io, common, surface, features, geometry, registration, visualization, sample consensus and in (one of the) apps.

Let's limit this PR

Eh, agreed, seems to me like ~130 files are a tad too many to check by hand. Unless IWYU can handle it all? Then I add the new header instead of replacing pcl_macros, so it's at most a useless include which was already present and IWYU would/should remove it with a pass. I can commit that and maybe Kunal could try a pass on the PR with it to check?

EDIT:

I'm not sure if global replace of pcl_macros.h with eigen_base.h is reasonable.

RE: global replace: it's actually replacing only the includes in the files with the custom allocation macro, to be clear. Still like this the scope is unrealistic, some pcl macro is bound to be needed by at least some of those files.

EDIT2:
I'd use the same files to add the new header (as long as a name is chosen) with only the allocation macro included, but ready for another PR targeting the other eigen macros. BTW apologies for the noise in the second commit but my text editor strips extra spaces.

[aPonza]

The errors don't seem related to the PR:

test 84
        Start  84: outofcore_test

84: Test command: /__w/1/build/test/outofcore/test_outofcore
84: Test timeout computed to be: 10000000
84: [==========] Running 16 tests from 2 test cases.
84: [----------] Global test environment set-up.
84: [----------] 4 tests from PCL

  [...]

84: [----------] 4 tests from PCL (837 ms total)
84: 
84: [----------] 12 tests from OutofcoreTest
84: [ RUN      ] OutofcoreTest.Outofcore_Constructors
84: unknown file: Failure
84: C++ exception with description "boost::filesystem::create_directory: No space left on device: "treeA/7/6/7/7/1/1/7/3/7/2"" thrown in the test body.
84: [  FAILED  ] OutofcoreTest.Outofcore_Constructors (14162 ms)
84: [ RUN      ] OutofcoreTest.Outofcore_ConstructorSafety
84: /__w/1/s/test/outofcore/test_outofcore.cpp:482: Failure
84: Value of: boost::filesystem::exists (filename_otreeB)
84:   Actual: false
84: Expected: true
84: No tree detected on disk. This test will fail. Perhaps this test was run out of order.
84: 
84: [  FAILED  ] OutofcoreTest.Outofcore_ConstructorSafety (0 ms)
84: [ RUN      ] OutofcoreTest.Outofcore_ConstructorBadPaths
84: [pcl::outofcore::OutofcoreOctreeBaseNode] Could not find dir treeBogus
84: [pcl::outofcore::OutofcoreOctreeBase] Wrong root node file extension: .bad_extension. The tree must have a root node ending in .oct_idx
84: [       OK ] OutofcoreTest.Outofcore_ConstructorBadPaths (0 ms)

  [...]

84: [----------] 12 tests from OutofcoreTest (16765 ms total)
84: 
84: [----------] Global test environment tear-down
84: [==========] 16 tests from 2 test cases ran. (17602 ms total)
84: [  PASSED  ] 14 tests.
84: [  FAILED  ] 2 tests, listed below:
84: [  FAILED  ] OutofcoreTest.Outofcore_Constructors
84: [  FAILED  ] OutofcoreTest.Outofcore_ConstructorSafety
84: 
84:  2 FAILED TESTS
 84/115 Test  #84: outofcore_test .........................***Failed   18.05 sec

[kunaltyagi]

but my text editor strips extra spaces.

Mine too (Don't know if I should cry or be happy about this)

[taketwo]

OK, the taken "global replace" approach seems solid to me and I'm not afraid that something is going to break.

However, and please forgive me for being stubborn, I do not understand the motivation behind:

• extracting this single macro into a file of it's own (PCL_MAKE_ALIGNED_OPERATOR_NEW is a macro, so logically belongs to pcl_macros.h I think);
• name choice for this header file (which meaning does "base" carry in this name?)

[kunaltyagi]

IMO, pcl_macros is a bad example of a filename. There are several kinds of macros and each brings with it a different header baggage. There are mainly 4 kinds of macros in PCL

• Platform/Compiler support macros: PCL_DEPRECATED("This stuff is bad for you"), PCL_FALLTHROUGH, PCL_EXPORTS etc.
• Library support macros: PCL_MAKE_EIGEN_TYPES_WORK (the macro in question), and others
• Quality of code support macros: PCL_ADD_NORMAL_4D, etc.
• Tooling macros: provided by CMake (in pcl_config.h), built on spot (for OpenMP, SSE, etc.)

Clearly, the last 2 types have been segregated by origin, use and functionality. The filename isn't a good argument for putting all macros everywhere in one file. Macros are just like functions. There's no harm in creating one file for a function if it doesn't fit in with 10 functions with a similar input arguments but different functionality.

[aPonza]

extracting this single macro into a file of its own

While there would be no harm in doing this per se (some go so far as to advocate for one function per file to avoid including unnecessary headers, even...a bit too much for me), my intention was to predispose a file to separate this macro together with the ones in point_types.hpp which were previously included. Doing so in two separate PRs allows a more granular check for breakage (as you say yourself, this micro-move I've made made you "not afraid that something is going to break", which was a concern for me as well with how the PR was set up 2 days ago).

If you think the PCL_MAKE_EIGEN_TYPES_WORK macros should be bundled together (e.g. to include Eigen/Core only there, but also to group logically similar macros together) then it's just a matter of choosing a fitting name for the header, which brings me to

which meaning does "base" carry in this name?

None really, maybe eigen_support.h, eigen_types.h (we use the macro to define an allocator trait, and the eigen vector maps are types too), or anything else that works better. My point in pushing with eigen_base.h was to at least confirm nothing breaks and the methodology is sound. It doesn't take much to change the name once one is decided. This PR is still a draft, if you will.

Moreover anybody that uses IWYU can already check the modified headers, where they might find some #include <pcl/pcl_macros.h> can be removed because the prior inclusion was only needed for PCL_MAKE_ALIGNED_OPERATOR_NEW.

[taketwo]

This PR started like a simple fix (move the include up, remove undefs) and in a blink of an eye spiraled into a massive changeset with over 100 files touched. On top of that, there is a parallel PR by Kunal. I was overwhelmed.

Now with your clarifications and extended opinions, I have a better understanding of your ideas, thank you. I do agree that there are different kinds of macros and that there is logic in being more granular and organizing them in separate files. I would support a principled reorganization of the headers. But I feel like we are missing some rough plan of where we are going.

[kunaltyagi]

On top of that, there is a parallel PR by Kunal

Sorry about my PRs. I've been moving slowly on them because they aren't automatic fixes and finicky compilation issues (see the 3 week old PR on grabber) really hamper productivity. The PR referred to consists of reorganization of the includes, not the headers and not other modules (beyond making things compile). As such, I did the bare minimum and would prefer another PR to attack the issue better.

I would support a principled reorganization of the headers. But I feel like we are missing some rough plan of where we are going.

One problem that I just realized is that PCL doesn't define which macros are public and which are private. That'd be the first step in making a transition plan.

To clarify my thoughts about this PR

• target: remove Eigen header from pcl_macros
• Why? pcl_macros is getting large and affecting multiple PR, implying it needs refactoring
• process:
  • move Eigen based macros to a new file: name? location? private/public?
  • update other files to include the new header (100+ files need updates)
• bonus:
  • move all macros that might be used with eigen together (not needed, but will help in making decisions both now and later (like filename, location, etc.))
  • remove pcl_exports and update includes (not needed here since I have done that in a separate commit and can create a PR of it once this PR is accepted)
• related PR:
  • adding index_t since adding new type to pcl_macros doesn't make sense (I added one type shared_ptr, but I now know that was an error)
    • this was started because there's already a large PR related to indices' type and I thought it was a nice time to do something that might help the original author to be future compatible
  • update #includes using IWYU since it keeps getting confused between the 2
    • the OG PR that started this PR

The other 2 PR will be mostly merged after a decision has been made on this one since

• they still need work (compilation, discussion, etc.)
• this PR is less broad (no larger reorganization, 'only' 800 lines of diff) as of now

As such any changes made in the parallel PR wrt pcl_macros can be ignored as I'm making them to keep the direction of my PRs consistent (not necessarily with this one).

[taketwo]

target: remove Eigen header from pcl_macros

👍

• move Eigen based macros to a new file: name? location? private/public?
• move all macros that might be used with eigen together (not needed, but will help in making decisions both now and later (like filename, location, etc.))

I guess have a different view on Eigen-related macros. We don't have many of them. There is PCL_MAKE_ALIGNED_OPERATOR_NEW in "pcl_macros.h" and there is a family of PCL_ADD_EIGEN_MAPS_xxx in "point_types.hpp"

• The former is Eigen-related, yes. However, it's also make_shared-related. In fact, the users who don't care about using pcl::make_shared don't have any reason to use this macro instead of the standard one. Therefore, I think it should instead go together with shared_ptr, make_shared, etc. Perhaps STL-like "memory.h" is an appropriate name choice.
• I don't see much benefit from moving the latter. These macros are helpers for creation/registration of PCL point types and are only used in "point_types.hpp". They are not exactly "private" because potentially the user may create his own point type and add these maps. However, the user will use them together with other stuff from "point_types.hpp", so it's not like he will be granular and include "eigen_base.h", but not "point_types.hpp".

[aPonza]

Ready for merge, unless you first want to fix 19.10

[SergioRAgostinho]

@kunaltyagi shouldn't the label be "needs: review"?

[kunaltyagi]

Sorry about that. I hope to get fewer errors as days go by

[kunaltyagi]

@aPonza Could you resolve the conflicts please?

[aPonza]

This is #2601 and #3722

[kunaltyagi]

Needs merge, not for 1.10.1

[SergioRAgostinho]

@PointCloudLibrary/maintainers Since we've decide to cherry pick for 1.10.1 can we merge this now?