#216 Merged at e913301
Repository
Branch
inline_namespace_attempt2
Repository
Branch
ign-math4
Author
  1. Shane Loretz
Reviewers
Description

Follow up from today's meeting, this PR adds versioned inlined namespaces. Major versions of ignition math can be linked into the same executable because the symbol names between two major versions no longer overlap.

More info on past PRs

  • Commit status

Comments (22)

  1. Shane Loretz author

    Note, bumped into a drawback: Ignition math types can't be forward declared by user code. As a workaround if a user wants to reference a math type they must include the header.

    This code is in ignition rendering

    namespace ignition
    {
      // forward declaration
      namespace math
      {
        class Color;
      }
    
      namespace rendering
      {
        class RenderTarget
      /// ...
          public: virtual math::Color BackgroundColor() const = 0;
    

    But it fails to build with the error

    In file included from /workspace/ws_ign/src/ign-rendering/include/ignition/rendering/base/BaseRenderTarget.hh:22:0,
                     from /workspace/ws_ign/src/ign-rendering/include/ignition/rendering/base/BaseCamera.hh:32,
                     from /workspace/ws_ign/build_isolated/ign_rendering/include/ignition/rendering/base/base.hh:5,
                     from /workspace/ws_ign/src/ign-rendering/src/base/BaseScene.cc:27:
    /workspace/ws_ign/src/ign-rendering/include/ignition/rendering/RenderTarget.hh:80:23: error: reference to 'Color' is ambiguous
           public: virtual math::Color BackgroundColor() const = 0;
                           ^
    /workspace/ws_ign/src/ign-rendering/include/ignition/rendering/RenderTarget.hh:32:11: note: candidates are: class ignition::math::Color
         class Color;
               ^
    In file included from /workspace/ws_ign/install_isolated/ign_common/include/ignition/common0/ignition/common/Material.hh:24:0,
                     from /workspace/ws_ign/src/ign-rendering/include/ignition/rendering/Scene.hh:23,
                     from /workspace/ws_ign/src/ign-rendering/include/ignition/rendering/base/BaseArrowVisual.hh:21,
                     from /workspace/ws_ign/build_isolated/ign_rendering/include/ignition/rendering/base/base.hh:3,
                     from /workspace/ws_ign/src/ign-rendering/src/base/BaseScene.cc:27:
    /workspace/ws_ign/install_isolated/ign_math/include/ignition/math4/ignition/math/Color.hh:35:33: note:                 class ignition::ma
    th::v4::Color
         class IGNITION_MATH_VISIBLE Color
                                     ^
    

    The forward declaration created a type ignition::math::Color, while the true type is ignition::math::v4::Color. Both can be referred to as ignition::math::Color so the compiler doesn't know which one to pick.

      1. Michael Grey

        Sorry for being late to this conversation, but I just stumbled across this thread now.

        Forward declarations can be very useful, and I don't think we should shrug off their use (there are many things about Google's C++ style guide that I strongly disagree with, and this is one of them).

        Instead, the conventional practice is to have your library provide a Types.hh header which forward-declares all of your library's interface types. We provide Types.hh headers in some places, but I don't think we've done a reliable job of keeping them up-to-date or comprehensive.

        My recommendation would be that, at some point, we do an audit of each ignition library to carefully examine the interfaces of each library (not just to figure out what we should forward-declare, but also because it would be valuable for cleaning up our code in general). During that audit, we can create the Types.hh header for each project.

  2. Steven Peters

    homebrew build failed, possibly because of using PairInput = uint32_t; at Helpers.hh:741

    Undefined symbols for architecture x86_64:
      "ignition::math::v4::Pair(unsigned int, unsigned int)", referenced from:
          HelpersTest_Pair_Test::TestBody() in Helpers_TEST.cc.o
      "ignition::math::v4::Unpair(unsigned long long)", referenced from:
          HelpersTest_Pair_Test::TestBody() in Helpers_TEST.cc.o
    ld: symbol(s) not found for architecture x86_64
    
    1. Shane Loretz author

      Fixed by c51c220. For some reason clang was interpreting the declaration definition as a brand new declaration in an unversioned namespace. Putting it into the versioned inline namespace fixed the issue.