The Wayback Machine - https://web.archive.org/web/20201017213618/https://github.com/sofa-framework/sofa/issues/39
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Long nested namespace and source code readability & complexity #39

Open
damienmarchal opened this issue Oct 19, 2016 · 8 comments
Open

Long nested namespace and source code readability & complexity #39

damienmarchal opened this issue Oct 19, 2016 · 8 comments

Comments

@damienmarchal
Copy link
Contributor

@damienmarchal damienmarchal commented Oct 19, 2016

According to the Sofa guidelines:
https://github.com/sofa-framework/sofa/blob/master/CONTRIBUTING.md
we have the following rule:
"You must avoid the using directive in header files (.h and .inl): using namespace foo."
The reason for this rule is that doing so generates namespace pollution which increase the risk of name collision. The drawback is that the long nested namespaces prefixes are repeated all around in the .h and .inl which impact readability.

What do you think of changing the rules and allowing to use "using namespace" and 'using' in the following context:

namespace sofa
{
    namespace component
    {
         namespace engine
         {
                 namespace boxroi     // A per file namespace 
                 {
                          using sofa::defaulttype::Vec3 ;
                          using namespace sofa::whatever 
                          class BoxRoi 
                          {
                                         // Here i can use Vec3 instead of sofa::defaultype::Vec3 or defaulttype::Vec3
                          };
                 }

                using boxroi::BoxRoi ;         // declaring only the BoxRoi in the engine namespace to 
                                                           // the sofa current namespace structure.. 
         }
    }
}

With such design the names imported in the boxroi namespace by 'using' and 'using namespace' are not leacking in the engine/component/sofa namespace.

I see no side-effect of such design but maybe you do.

DM.

@matthieu-nesme
Copy link
Member

@matthieu-nesme matthieu-nesme commented Oct 21, 2016

Personally I am not disturbed by expliciting namespaces, at least the code is a bit easier to follow. 'using namespace' is somehow hiding information.

Also remember that you can add typedef inside your class.
E.g.
class BoxRoi { typedef sofa::defaulttype::Vec3 Vec3; // Here i can use Vec3 instead of sofa::defaultype::Vec3 or defaulttype::Vec3 };

Maybe is it possible to do trickier things with c++11???!

@untereiner
Copy link

@untereiner untereiner commented Oct 21, 2016

Or replace typedef keyword by using keyword for the type as c++11 is allowed:
class BoxRoi { using Vec3 = sofa::defaulttype::Vec3 };

@damienmarchal
Copy link
Contributor Author

@damienmarchal damienmarchal commented Oct 24, 2016

I all,

On my side I think that the 'using' directive (the classical 'using namespace': http://en.cppreference.com/w/cpp/language/namespace) is really bad and is hiding information (It is in fact similar to the python "from xx import *"), for this reason is should be forbidden into the .h and .inl.

But it also seems to me that the situation of the 'using' declaration (http://en.cppreference.com/w/cpp/language/namespace) as well as the 'using' type alias (http://en.cppreference.com/w/cpp/language/type_alias) is really different. As they are 'explicit' they do not hide anything and we can control their visibility as python developpers are doing with the "from xx import MyVec as Vec3".

So given the fact that the latter two are:

  • not hiding information
  • does not leak name into namespaces
    who is to forbid/allow their use in the sofa header files and should we update the Sofa guidelines to take them into account ?

DM.

@JeremieA
Copy link
Contributor

@JeremieA JeremieA commented Jan 10, 2017

Many years ago during a technical meeting we made two decisions that were never fully implemented :

  1. API classes ( in sofa::core, sofa::helper, ...) should only use a two-level hierarchy, with grouping documented as doxygen comments for example but not through deep namespaces
  2. component classes (sofa::component::* ) would be moved to namespaces matching the name of the plugin in which they are, in order to match their new include path

But unfortunately it was low priority and not put into work... yet ;)

@damienmarchal
Copy link
Contributor Author

@damienmarchal damienmarchal commented Jan 10, 2017

Maybe doing

namespace sofa
{
namespace core
{
      using sofa::core::verydeeptnamespace::oldnamingscheem::AClass ; 
} 
}

Would allow to expose the old namespace prefix to the new scheme without the need to change the existing code base.

This could help during a transitional period because the existing code wouldn't need to be changed at a single time.

DM.

@JeremieA
Copy link
Contributor

@JeremieA JeremieA commented Jan 10, 2017

That's a good idea! The same could be done for the include path, temporarily keeping sofa/core/xx/yy.h which would just be a one-liner #include <sofa/core/yy.h>. Or doing the opposite, which could make merging existing codes easier (at the cost of requiring the move later, and not having the actual code in the files that are supposed to be the new correct location now).

Regarding the earlier point about using using in the class itself, I know that one previous bothersome limitation was that the syntax for the return types of methods that are not defined within the class would still require defining the complete type, leading to codes like :

  • template<class T> typename T::VecCoord MyComponent<T>::mymethod(VecCoord v)
  • template<class T> very::long::namespace::MyType MyComponent<T>::mymethod(MyType a)
  • template<class T> typename MyComponent<T>::MyType MyComponent<T>::mymethod(MyType a)

But now with c++11, if VecCoord and MyType are typedef/using within the class, we can do :

  • template<class T> auto MyComponent<T>::mymethod(VecCoord v) -> VecCoord
  • template<class T> auto MyComponent<T>::mymethod(MyType a) -> MyType

A good improvement :)

@damienmarchal
Copy link
Contributor Author

@damienmarchal damienmarchal commented Jan 11, 2017

I tried the private namespace trick + 'using' to support backward compatibility in
I tried the combination of private namespace and 'using' in
https://github.com/SofaDefrost/sofa/blob/341330312c267ce5695bbd23f7ad7a8ef2b11c37/SofaKernel/modules/SofaBaseVisual/VisualModelImpl.h

Feedback welcome.

@damienmarchal
Copy link
Contributor Author

@damienmarchal damienmarchal commented Mar 24, 2017

Hi all

I would be very happy if at the STC we re-warm the decision about having a clear naming scheme.
So if some of you are interested in this topic I suggest to book for one of our cofee break next week :)

My current position on the issue is that:

  • it become more and more messy as we have no 'applied' rule.

To me there is three interesting feature namespace should have each grounded by different needs:
- namespace that mimick path.
- namespace that group code logically.
- namespace that are short to write/read.

Since c++11 it is very possible to have all that with a dual namespace scheme in which one namespace match the files location. But as this namespace is in general long we have a second namespace organization that export the names in a more accessible namespace.

Ex (for a class in sofa/somehwere/again/MyClass.h):

    /// namespace (hierarchical grouping)
    namespace sofa {
           namespace somewhere {
                namespace again {
                      namespace __myclass__ {
                            /// This is a private namespace that I like a lot :) 
                            /// because we can import things we want with name leacking all around
                            /// No more typedef when they are not needed :) 
                            /// The name here are available in __myclass__ (so normally only 
                            /// in the .cpp and .h and .inl) 
                            using std::vector; 
                            using sofa::defaultype::Vec;
                            using sofa::suplercomplex::longns::OtherClass ; 

                            class MyClass : OtherClass
                            {
                                         MyCLass(Vec<3,float>&, ...) ; 
                            };
}
}
}
}

Then come the logical grouping in which the MyClass is exported into its logical group with 'using'

    /// The second namespace schema (logical grouping)
    namespace sofa {
           namespace component {
                  namespace collision {
                          using sofa::somewhere::again::__myclass__::MyClass ;
                  }
           }
    }

So generally I such approach that this increase a lot the readability of the source code and conceptually this gives a taste of how python is handling import of modules and avoid nameclash.

@damienmarchal damienmarchal added this to Proposal in Code Improvement Mar 30, 2017
@damienmarchal damienmarchal moved this from Proposal to Todo in Code Improvement Sep 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.