Check if classes are in correct namespaces

[SunBlack]

During dumping symbols of a PCL library, I saw some classes are not in correct namespace.

Example: DifferenceOfNormalsEstimation is only in namespace pcl and not in pcl::features like it should, because this class is part of module pcl_features.

Fastes way to find this "bad" classes should be using (MSVC example)

dumpbin /exports  pcl_features_release.lib

And then search for all lines which does not contain e.g. pcl::features.

This would be a breaking change! Maybe this is a good time to do so, because of switch to C++14 it is maybe better to change next version number from 1.10 to 2.0 (even C++14 alone is a reason for me to do so ;) ).

[taketwo]

Enabling C++14 flags is not breaking API, thus does not warrant for major version bump.

Historically most of the classes are in the root pcl namespace. Some are in nested namespace. Yes, this is not consistent. Should we change this? In my opinion, no. We will gain close to nothing by shuffling classes around, but will piss off lots and lots of people by breaking their code.

[SunBlack]

Enabling C++14 flags is not breaking API, thus does not warrant for major version bump.

True, but some old compilers won't work anymore, so you can count it as major change and currently there are a lot of changes to PCL (e.g. regarding boost). So wanted only say maybe it is a good time to do breaking changes since there are/will be a lot of changes internally.

Historically most of the classes are in the root pcl namespace. Some are in nested namespace. Yes, this is not consistent. Should we change this? In my opinion, no. We will gain close to nothing by shuffling classes around, but will piss off lots and lots of people by breaking their code.

True, but now we piss off new users, if they see a class in a module, but they have always to look into documentation/code in which namespace it is.

[SunBlack]

I have an idea how we can change this (not tested):

Before

namespace pcl
{
  class DifferenceOfNormalsEstimation ...
}

After

namespace pcl
{
  namespace features
  {
    class DifferenceOfNormalsEstimation ...
  }
  using DifferenceOfNormalsEstimation [[deprecated]] = features::DifferenceOfNormalsEstimation;
}

[taketwo]

Thanks to C++11, this approach will work (before we would need to use typedef and it does not support template parameters).

That said, I'm still in favor of maintaining status quo. The change you proposed will result in 90% of the library classes and functions marked as deprecated. Just imagine how much effort it will be for existing users to update all their projects...

[SunBlack]

Just imagine how much effort it will be for existing users to update all their projects...

As long as you don't treat deprecation warning as error, it doesn't hurt. And for new users it's better if we fix it.

Currently I'm waiting for #2376, so I don't need to fix indention manually (this is one thing I don't like about PCL style guide. indention of namespace xD)

[taketwo]

As long as you don't treat deprecation warning as error, it doesn't hurt.

This being a good practice, I would assume a lot of people do treat warnings as errors. Then they need to update their flags to exclude deprecation warnings. And then their build output is still flooded with deprecation warnings... This is a mess.

And for new users it's better if we fix it.

Is it? As I mentioned earlier, most of PCL stuff is in the root namespace. By default, if you a thinking about a function or a class, it will be in pcl::. Off the top of my head, exceptions are only io:: and visualization::. Not so much trouble to learn this. And most people use IDEs nowadays, which helps.

[SunBlack]

And for new users it's better if we fix it.

Is it? As I mentioned earlier, most of PCL stuff is in the root namespace. By default, if you a thinking about a function or a class, it will be in pcl::. Off the top of my head, exceptions are only io:: and visualization::. Not so much trouble to learn this. And most people use IDEs nowadays, which helps.

It is as long as it is not consistently. It doesn't matter if we have always pcl::<module>::<class> or pcl::<class> - just mixing it is ugly.

[SergioRAgostinho]

Once more late to this party.

I tend to prefer pcl::<class>, as I like to explicitly write the namespace to quickly identify "what is coming from where". pcl::<namespace>:: becomes cumbersome quickly. That being said, two comments linger in the back of my mind that would require checking:

• There was a thread regarding pcl::io::<classes> naming convention that I've seen @jspricke mentioning. It might be worth revisiting that.
• I believe there are classes in the cuda, gpu modules which have literally the same name as the non GPU version. I like to have the pcl::gpu namespace for these cases, but we can also suffix it with GPU at the end.

Regarding pissing off-users, I'm not sure on how to address this. Once we decide on a convention I believe I would first created all required "aliases" so that users could employ the new naming convention once it's settled. After that I would start deprecating the more obscure modules first. Maybe one module every six months?

Anyway, this is very low priority for me.

[Morwenn]

Late point of view: the pcl::<namespace>::, if applied consistently, could really help discoverability. Knowing that feature from module MMM lives in pcl::mmm:: means that we don't have to check over and over again all the time, which is convenient. It would also make it obvious which components would appear/disappear when passing a BUILD_mmm flag to CMake, and very easy to check what parts of the code would be affected with a simple grep pcl::mmm command.

I was bitten recently because the grabbers don't all live in the same namespace. For these components specifically I wouldn't mind them all being in pcl::io:: because the nested namespace names are short and I generally write a grabber's class name once or twice and then just use member functions on an instance, so adding it there isn't bothersome.

Now I'm also sympathetic with the fact that common function and class names are more easily accessible if they live directly in pcl:: - having to repeat some names over and over again is sometimes annoying, but that's also why C++ has using directives to import common names in the local scope.

I think that a first step would be to put all the affected components in their module's namespace and to provide aliases to avoid breaking the current API. It would allow people who wish to fully qualify names - for any of the reasons I mentioned in the first paragraph - to do so, and wouldn't break any code. If you don't want to break ABI either it can be done the other way around by making the subnamespace-level features aliases of the ones in pcl::. Deprecation can come later if needed or never.

[kunaltyagi]

I don't know how important this party is 🎉 , but we can take a look at other libraries:

• Boost: shallow nesting. Few, small, common names are in boost::, others are in boost::module
• OpenCV: mixed. Core functions are in cv::, specialized/contrib are in cv::module
• Poco: mixed. Common functionality in poco::, specialized in poco::module

For large implementations, we have

• Microsoft: deep nesting. Similar to the .NET framework
• stdlib: nearly flat, specializations in std::module

Based on this, the core of PCL should not be in a namespace. Specialized use/impl can be sent in pcl::module. A reorganization of modules would be most helpful here. The best example I can think of are code relying on a platform specific API. To name a few:

• cuda
• kinfu
• openni

In other cases such as for KdTree, pcl::kdtree::KdTree doesn't make much sense since the name of class and module name are similar (stuttering is not ideal to type). Creating an 'acceleration' namespace for all acceleration data structures would be better.

Some consideration should be given to how "healthy" a namespace is. There should be sufficient items to merit a namespace. (Should there be an "bloatedness" concept too? Maybe no) Of course, if there is any scope of confusion, it's better to have a namespace to disambiguate the meaning.

We should also strive to keep the class names similar across namespaces. pcl::XYZ and pcl::gpu::XYZ_gpu don't really help the user, since pcl::XYZ and pcl::gpu::XYZ would provide a nicer overall API. In the same vein, I think pcl::openni::Grabber would make more sense than pcl::io::OpenNiGrabber (since it should be easy to tell what's available and what's not by looking at some compile definitions/macros provided by PCL)

As a library, it shouldn't matter if things are difficult for the authors. As a library, it is a sort of contract to make things as easy as possible for the end-user. If that means using namespaces, use it (aka Microsoft for developers familiar with .NET) and if it means not using namespaces, don't use (stdlib). If each module is independent then let each module have a namespace (aka Boost). Large number of module_a::XYZ in module B would imply they need better cohesion or reorganization.

The approach of std::chrono::literals should also be looked at. The same types are available via multiple locations. This implies that we can provide the same type via pcl::stereo::io::XYZ as well as pcl::io::XYZ if need be.

I have no stand on how to proceed with the issue, just some observations that might help.

[Morwenn]

Concerning Boost and the standard library there are a few more things to consider (once again it's just informative, not a proposal):

• Boost is becoming so big that it sometimes suffers from shallow nesting from time to time and wish some of its components were deeply nested. For example boost::sort is both an algorithm and a namespace, and you can't use both together (issue boostorg/sort#35). Now PCL fortunately isn't on the scale of Boost, so clever naming can mostly avoid issues (and even something the size of Qt apparently manages too).
• The standard library have both proponents and opponents of nested namespaces, which P2065 sums up quite well - it also proposes solutions but hasn't been discussed by the committee yet, so waiting for conclusions could be an option. It's worth noting that the nested namespace hate has transformed most of std::fmt::* into std::format_*, which is just C-style namespacing. The namespacing of UDLs is itself a different yet elegant design to import make sure they can be imported into the current namespace with a good enough control over the granularity of the import - but those are meant to be used with using namespace so the design makes sense wrt to that.

[taketwo]

Thanks for the discussion. Clearly, there are numerous valid arguments in favor of both approaches. If we were to start writing this library from scratch, it wouldn't be an easy decision. However, the reality is that we have a mature code base. In my opinion, this tips the balance towards flat namespacing as this is what we (mostly) already have.

[stale[bot]]

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.

[kunaltyagi]

Shall we consider the discussion as over and the conclusion as "maintain status quo"?

[SergioRAgostinho]

I'm ok with that decision.