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

Avoiding include and identifier collision #36

Closed
mosra opened this Issue Dec 16, 2013 · 4 comments

Comments

3 participants
@mosra
Owner

mosra commented Dec 16, 2013

Currently Magnum (and also Corrade) headers are included with no explicit path or prefix. They also have very generic names:

#include <Color.h>

Generic, short and unprefixed names are good for usability, but only until some issue appears. Notably on Windows (both with MinGW and MSVC) there are name collision problems, e.g. Types.h from motionblur example conflicts with Types.h from Magnum, causing horrific error messages. One other example was String.h from Corrade Utility, which was conflicting with system string.h, but that issue disappeared somehow. There are also other problems, which may or may not be bugs in how the compiler handles paths and filenames. It's not limited to files though -- the Magnum::Rectangle class conflicted with Rectangle() from WINAPI, but that's not an issue anymore, as Rectangle is now replaced with more generic Range. In this case the <windows.h>/<Xlib.h> headers with unprefixed functions and macros are the main offenders, but I can't do anything else than accomodate to them.

One simple solution to include collision might be to not add (e.g.) /usr/include/Magnum to include path and use absolute includes instead, i.e.:

#include <Magnum/Color.h>

This would require fairly big repository reorganization -- moving everything from src/ to src/Magnum and then changing all headers to have absolute includes too. The includes from external directory would need to be adjusted too. Then the src/ directory would contain only one Magnum subdirectory, resembling the very verbose and inconvenient way Java projects are organized in the filesystem.

This change has also its downsides, mainly:

  • The includes are way more verbose. Not so much for root includes, but for subnamespaces:

    #include <Magnum/Math/Algorithms/GaussJordan.h>
    
  • Unwanted separation of Corrade (sub)library. Currently Corrade library is a first-class citizen in Magnum, i.e. the user doesn't have to think whether some class is part of Magnum or Corrade, it's enough just include it and use as if it was part of Magnum:

    #include <Magnum.h>
    #include <Utility/Directory.h>
    
    using namespace Magnum;
    
    Directory::write(...);
    

    With this change the user would need to explicitly specify the actual project given header comes from, which is not so convenient:

    #include <Magnum/Magnum.h>
    #include <Corrade/Utility/Directory.h> // huh?
    
  • To allow multiple versions of the library be installed alongside each other, the includes would need to be installed into (e.g.) include/Magnum1/Magnum and include/Magnum2/Magnum, which is even more horrifying.

I hope for a less drastic solution than this.

Another solution might be to have the headers and classes with some unique prefix. If I take inspiration from Qt, they are having everything in Qt*/ subdirectories, each class is prefixed with Q* and each function is prefixed with q*. But (as far as I know) this was from the times where support for C++ namespaces was rather poor (Qt classes are not wrapped in any namespace) and with current C++ state the final decision might be totally different.

I thought about adding similar prefix to the root classes and subnamespaces (Mn*, Cr*), but I don't like having Magnum::MnSceneGraph, Magnum::MnVector2 etc., it seems overly verbose and redundant for me. It might be possible to have the prefix only for header files, but having to include <MnRenderer.h> to use Magnum::Renderer is not intuitive at all.

@Jmgr

This comment has been minimized.

Show comment
Hide comment
@Jmgr

Jmgr Dec 17, 2013

In my projects I always use absolute filenames when including files from external libraries, but I would find it better if the library itself would enforce this.
For me a good example of a well-designed library is Boost. Every library in Boost has its own directory (except some very old headers). I think Boost is a way better example than Qt concerning that. I think that prefixes are really obsolete and confusing in modern C++. (It also reminds me of the much feared Hungarian notation!)
As for the issue with the separation of Corrade and Magnum, I think that "using namespaces" are pure evil, so I am used to look at the documentation to know were a specific functionality resides. As for having different versions of the libraries, I suppose you could just use another prefix? I'm not sure if this is a very common use case.

Jmgr commented Dec 17, 2013

In my projects I always use absolute filenames when including files from external libraries, but I would find it better if the library itself would enforce this.
For me a good example of a well-designed library is Boost. Every library in Boost has its own directory (except some very old headers). I think Boost is a way better example than Qt concerning that. I think that prefixes are really obsolete and confusing in modern C++. (It also reminds me of the much feared Hungarian notation!)
As for the issue with the separation of Corrade and Magnum, I think that "using namespaces" are pure evil, so I am used to look at the documentation to know were a specific functionality resides. As for having different versions of the libraries, I suppose you could just use another prefix? I'm not sure if this is a very common use case.

@miguelmartin75

This comment has been minimized.

Show comment
Hide comment
@miguelmartin75

miguelmartin75 Dec 23, 2013

Contributor

I think that "using namespaces" are pure evil

@Jmgr
In my opinion, that's only the case in header files (specifically within global scope; and even in global scope in source files as there may still be name collisions).

For me a good example of a well-designed library is Boost. Every library in Boost has its own directory (except some very old headers). I think Boost is a way better example than Qt concerning that. I think that prefixes are really obsolete and confusing in modern C++. (It also reminds me of the much feared Hungarian notation!)

@Jmgr
+1 I fully agree.

Unwanted separation of Corrade (sub)library. Currently Corrade library is a first-class citizen in Magnum, i.e. the user doesn't have to think whether some class is part of Magnum or Corrade, it's enough just include it and use as if it was part of Magnum:

#include <Magnum.h>
#include <Utility/Directory.h>

using namespace Magnum;

Directory::write(...);

With this change the user would need to explicitly specify the actual project given header comes from, which is not so convenient:

#include <Magnum/Magnum.h>
#include <Corrade/Utility/Directory.h> // huh?

@mosra
It may not be "convenient" in your words, but handling directories (i.e. doing what corrade does) is not apart of the magnum library. It is apart of the Corrade library, which is required to be installed for magnum, but the user does not require to (explicitly) use Corrade in their code. As he/she may instead decide to use an alternative to Corrade, which he/she may be familiar with (such as boost). If the user wants to use Corrade, he/she should do so explicitly; the reason for being explicit is simple: it's more readable to do so, e.g. a 3rd party person observing one's code may not be sure what header file belongs to what library.

To allow multiple versions of the library be installed alongside each other, the includes would need to be installed into (e.g.) include/Magnum1/Magnum and include/Magnum2/Magnum, which is even more horrifying.

@mosra
Typically when working with SDL, it seems convention (at the moment, at least), to have the headers within two seperate directories. include/SDL (no number seems to be implicit for 1; which is convenient, since it's similar to algebra x == 1x) and include/SDL2, which seems to work quite fine. Note that to include the header files within the SDL2 library, you would write #include <SDL2/SDL.h>, which in my opinion is better than #include <SDL/SDL.h>, as it is more explicit for which SDL version you are using (which helps for looking up documentation and reading the code, as you know what version of the library you are using).

Contributor

miguelmartin75 commented Dec 23, 2013

I think that "using namespaces" are pure evil

@Jmgr
In my opinion, that's only the case in header files (specifically within global scope; and even in global scope in source files as there may still be name collisions).

For me a good example of a well-designed library is Boost. Every library in Boost has its own directory (except some very old headers). I think Boost is a way better example than Qt concerning that. I think that prefixes are really obsolete and confusing in modern C++. (It also reminds me of the much feared Hungarian notation!)

@Jmgr
+1 I fully agree.

Unwanted separation of Corrade (sub)library. Currently Corrade library is a first-class citizen in Magnum, i.e. the user doesn't have to think whether some class is part of Magnum or Corrade, it's enough just include it and use as if it was part of Magnum:

#include <Magnum.h>
#include <Utility/Directory.h>

using namespace Magnum;

Directory::write(...);

With this change the user would need to explicitly specify the actual project given header comes from, which is not so convenient:

#include <Magnum/Magnum.h>
#include <Corrade/Utility/Directory.h> // huh?

@mosra
It may not be "convenient" in your words, but handling directories (i.e. doing what corrade does) is not apart of the magnum library. It is apart of the Corrade library, which is required to be installed for magnum, but the user does not require to (explicitly) use Corrade in their code. As he/she may instead decide to use an alternative to Corrade, which he/she may be familiar with (such as boost). If the user wants to use Corrade, he/she should do so explicitly; the reason for being explicit is simple: it's more readable to do so, e.g. a 3rd party person observing one's code may not be sure what header file belongs to what library.

To allow multiple versions of the library be installed alongside each other, the includes would need to be installed into (e.g.) include/Magnum1/Magnum and include/Magnum2/Magnum, which is even more horrifying.

@mosra
Typically when working with SDL, it seems convention (at the moment, at least), to have the headers within two seperate directories. include/SDL (no number seems to be implicit for 1; which is convenient, since it's similar to algebra x == 1x) and include/SDL2, which seems to work quite fine. Note that to include the header files within the SDL2 library, you would write #include <SDL2/SDL.h>, which in my opinion is better than #include <SDL/SDL.h>, as it is more explicit for which SDL version you are using (which helps for looking up documentation and reading the code, as you know what version of the library you are using).

@mosra

This comment has been minimized.

Show comment
Hide comment
@mosra

mosra Dec 25, 2013

Owner

The using namespace is evil if it brings symbols into global namespace. In all cases I'm bringing names from one namespace to another. In Magnum.h I'm doing precisely this:

namespace Magnum {
    using namespace Corrade;
    using Corrade::Utility::Debug;
    using Corrade::Utility::Warning;
    using Corrade::Utility::Error;
}

And in my projects I'm often creating a "import header" with these contents, which is then included everywhere else, making Magnum namespace available in namespace of particular project.

namespace MyGame {
    using namespace Magnum;
}

This doesn't pollute the global namespace or cause any unsolvable naming conflicts. It is, however, entirely up to users whether they want to prefix everything with Magnum:: or not. I am strongly for using std:: for STL things, as it is short enough to not add much noise into the code, but Magnum:: is too long. There are also namespace aliases, e.g. namespace Mn = Magnum;, which might help with the noise, but on the other hand add confusion to everything.


As for the includes, my only remaining concern is that I need to move everything from src/ to src/Magnum so I can make also the inter-project includes absolute. In the distant future (when there will be some Magnum 2) I will need do the move again, from src/Magnum to src/Magnum2, even if the source changes would be more evolutional than revolutional.

This move would complicate Git history and file-specific Git logs and make src/ look like an redundant path level. Is there any way to avoid this (preprocessor options, faking include path)? There is also an option to move headers from src/ to include/Magnum and keep the sources where they are, but I would lose the source-header locality, which is very convenient when just browsing through the files.

Owner

mosra commented Dec 25, 2013

The using namespace is evil if it brings symbols into global namespace. In all cases I'm bringing names from one namespace to another. In Magnum.h I'm doing precisely this:

namespace Magnum {
    using namespace Corrade;
    using Corrade::Utility::Debug;
    using Corrade::Utility::Warning;
    using Corrade::Utility::Error;
}

And in my projects I'm often creating a "import header" with these contents, which is then included everywhere else, making Magnum namespace available in namespace of particular project.

namespace MyGame {
    using namespace Magnum;
}

This doesn't pollute the global namespace or cause any unsolvable naming conflicts. It is, however, entirely up to users whether they want to prefix everything with Magnum:: or not. I am strongly for using std:: for STL things, as it is short enough to not add much noise into the code, but Magnum:: is too long. There are also namespace aliases, e.g. namespace Mn = Magnum;, which might help with the noise, but on the other hand add confusion to everything.


As for the includes, my only remaining concern is that I need to move everything from src/ to src/Magnum so I can make also the inter-project includes absolute. In the distant future (when there will be some Magnum 2) I will need do the move again, from src/Magnum to src/Magnum2, even if the source changes would be more evolutional than revolutional.

This move would complicate Git history and file-specific Git logs and make src/ look like an redundant path level. Is there any way to avoid this (preprocessor options, faking include path)? There is also an option to move headers from src/ to include/Magnum and keep the sources where they are, but I would lose the source-header locality, which is very convenient when just browsing through the files.

@mosra

This comment has been minimized.

Show comment
Hide comment
@mosra

mosra Jan 11, 2014

Owner

I finally managed to change all the includes to absolute, the relevant commit set is from 45a10ce to 2222922 (and similarly named commits in Corrade and other projects). The changes were pretty drastic so please bear with me :-) All depending projects are also updated and the freshly uploaded documentation now reflects the "absolute state" too.

If building with MAGNUM_BUILD_DEPRECATED and CORRADE_BUILD_DEPRECATED it is still possible to use non-absolute includes (so some backwards compatibility is preserved), with non-deprecated build only absolute includes are allowed. To avoid issues it may be needed to recreate the build dir and also update FindCorrade.cmake and FindMagnum.cmake.

Owner

mosra commented Jan 11, 2014

I finally managed to change all the includes to absolute, the relevant commit set is from 45a10ce to 2222922 (and similarly named commits in Corrade and other projects). The changes were pretty drastic so please bear with me :-) All depending projects are also updated and the freshly uploaded documentation now reflects the "absolute state" too.

If building with MAGNUM_BUILD_DEPRECATED and CORRADE_BUILD_DEPRECATED it is still possible to use non-absolute includes (so some backwards compatibility is preserved), with non-deprecated build only absolute includes are allowed. To avoid issues it may be needed to recreate the build dir and also update FindCorrade.cmake and FindMagnum.cmake.

@mosra mosra closed this Jan 11, 2014

@mosra mosra added this to the 2014.01 milestone Feb 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment