Replace spurious forward declaration #144
Conversation
Previously the forward declaration referred to a non-existing `class U`.
|
I actually wonder, if this forward declaration ever had any influence on anything (except breaking compilation with g++7/g++8) |
|
Searching the past reveals (168671) that this code was inserted in order to pass tests with msvc 8.0. Have you tested your change with this compiler? What tests have you run with change. |
|
I did not test MSVC (since I don't have easy access to that). The patch does resolve the issue linked above (which actually seems to be a gcc bug). Also, the forward declaration seems in fact not have any effect at all on any gcc and clang version I tested (other than exposing the gcc bug in C++17 mode). |
|
Do you need any additional information from me? I won't be able to verify or falsify whether this is necessary or breaks anything with MSVC. It seems the AppVeyor build fails in the main repository as well, so I don't think this patch introduces any bug. I could elaborate on why I believe the existing forward declaration does not make sense, if you want me to. From Eigen's side this PR is actually not urgent/necessary, since we worked around the issue in another way (see the links above if you are interested). |
|
the problem is that if I don't re-test everything after every change we end up running in circles rather than moving forward. It ends up being a hugely time consuming game of whack-a-mole as "bug" reports come in whenever anything changes. So I'm particularly reluctant to back out a work around which was likely the result of a very long back and forth with some user(s) of some obscure platform. Of course this situation is made much, much worse by the fact that users depend upon being to load very old files created via serialization. Depending upon the change, I'm reluctant to incorporate something which hasn't been vetted on MSVC as this is huge source of surprises whenever a change is made. In practice I depend upon the boost test matrix. That is run a change on my machine, then check it in to develop, watch the boost test matrix: https://www.boost.org/development/tests/develop/developer/serialization.html and then eventually merge it into master branch for release. Currently the boost test matrix doesn't test msvc 8.0 anymore. So I might bite the bullet and check this in anyway. Still I have to think about it and have to defer it until I have a little free time. I'm leaving this open until I actually do something here. |
|
Totally understand your point. As I said, for me this is not urgent, so feel free to get back to it whenever you find time. An option might be to Thanks for maintaining this library! |
|
Note that test with gcc on Windows fails with this change. So maybe the forward declaration is required after all. You might want to investigate the specific failure. |
|
note that incorporating your change make tests fail when built with the static version of the library. This forward declaration was inserted to address some quirk in MSVC a long time back. So it seems that MSVC still has some quirk which is addressed by this forward declaration. Hence, we need to leave in. |
hpp-fcl v1.7.3 doesn't compile on the standard default configuration on
ArchLinux (GCC 11.1.0, Eigen 3.3.9, Boost 1.75.0) and on
Fedora 34 (GCC 11.1.1, Eigen 3.3.9, Boost 1.75.0), with the following
error message:
```
In file included from /usr/include/eigen3/Eigen/Core:368,
from ./hpp-fcl-1.7.3/include/hpp/fcl/data_types.h:41,
from ./hpp-fcl-1.7.3/include/hpp/fcl/collision.h:43,
from ./hpp-fcl-1.7.3/test/serialization.cpp:40:
/usr/include/eigen3/Eigen/src/Core/util/ForwardDeclarations.h: In instantiation of 'struct Eigen::internal::accessors_level':
/usr/include/eigen3/Eigen/src/Core/util/ForwardDeclarations.h:109:58: required by substitution of 'template class SPT> void boost::serialization::load(Archive&, SPT&, unsigned int) [with Archive = boost::archive::text_iarchive; SPT = ]'
/usr/include/boost/serialization/split_free.hpp:58:13: required from 'static void boost::serialization::free_loader::invoke(Archive&, T&, unsigned int) [with Archive = boost::archive::text_iarchive; T = Eigen::Map, 0, Eigen::Stride<0, 0> >]'
/usr/include/boost/serialization/split_free.hpp:74:18: required from 'void boost::serialization::split_free(Archive&, T&, unsigned int) [with Archive = boost::archive::text_iarchive; T = Eigen::Map, 0, Eigen::Stride<0, 0> >]'
./hpp-fcl-1.7.3/include/hpp/fcl/serialization/eigen.h:82:17: required from 'void boost::serialization::serialize(Archive&, Eigen::Map&, unsigned int) [with Archive = boost::archive::text_iarchive; PlainObjectBase = Eigen::Matrix; int MapOptions = 0; StrideType = Eigen::Stride<0, 0>]'
/usr/include/boost/serialization/serialization.hpp:109:14: required from 'void boost::serialization::serialize_adl(Archive&, T&, unsigned int) [with Archive = boost::archive::text_iarchive; T = Eigen::Map, 0, Eigen::Stride<0, 0> >]'
/usr/include/boost/archive/detail/iserializer.hpp:187:40: [ skipping 29 instantiation contexts, use -ftemplate-backtrace-limit=0 to disable ]
/usr/include/boost/archive/detail/common_iarchive.hpp:67:22: required from 'void boost::archive::detail::common_iarchive::load_override(T&) [with T = hpp::fcl::BVHModelBase; Archive = boost::archive::text_iarchive]'
/usr/include/boost/archive/basic_text_iarchive.hpp:70:52: required from 'void boost::archive::basic_text_iarchive::load_override(T&) [with T = hpp::fcl::BVHModelBase; Archive = boost::archive::text_iarchive]'
/usr/include/boost/archive/text_iarchive.hpp:82:52: required from 'void boost::archive::text_iarchive_impl::load_override(T&) [with T = hpp::fcl::BVHModelBase; Archive = boost::archive::text_iarchive]'
/usr/include/boost/archive/detail/interface_iarchive.hpp:68:36: required from 'Archive& boost::archive::detail::interface_iarchive::operator>>(T&) [with T = hpp::fcl::BVHModelBase; Archive = boost::archive::text_iarchive]'
./hpp-fcl-1.7.3/test/serialization.cpp:122:10: required from 'void test_serialization(const T&, T&, int) [with T = hpp::fcl::BVHModelBase]'
./hpp-fcl-1.7.3/test/serialization.cpp:231:23: required from here
/usr/include/eigen3/Eigen/src/Core/util/ForwardDeclarations.h:32:48: error: incomplete type 'Eigen::internal::traits' used in nested name specifier
32 | enum { has_direct_access = (traits::Flags & DirectAccessBit) ? 1 : 0,
| ^~~~~
/usr/include/eigen3/Eigen/src/Core/util/ForwardDeclarations.h:33:47: error: incomplete type 'Eigen::internal::traits' used in nested name specifier
33 | has_write_access = (traits::Flags & LvalueBit) ? 1 : 0,
| ^~~~~
```
This bug was already reported at https://stackoverflow.com/questions/54534047/eigen-matrix-boostserialization-c17
and discussed at https://gitlab.com/libeigen/eigen/-/issues/1676
A minimal reproducible example is provided at https://godbolt.org/z/uIy1Uu
This example shows that this bug is triggered only on GCC >= 7 (other
compilers are not affected) and on C++17 (which is the default for
GCC >= 11).
Also, hpp-fcl 1.7.3 compilation is fine on Arch either by using clang or
by explicitely setting GCC on C++14.
This commit implements the workaround provided by Christoph Hertzberg on
the Eigen issue discussion.
Another workaround is already available in Eigen >= 3.3.8:
https://gitlab.com/libeigen/eigen/-/commit/2aa9eb3ce8fa6b2d61dce5be9d6d6460a28080c4
But this doesn't fix the build of hpp-fcl 1.7.3.
Another workaround was proposed to boost::serialization, but was
rejected: boostorg/serialization#144
The main bug in GCC got no attention for the last 2 years:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84075
hpp-fcl v1.7.3 doesn't compile on the standard default configuration on
ArchLinux (GCC 11.1.0, Eigen 3.3.9, Boost 1.75.0) and on
Fedora 34 (GCC 11.1.1, Eigen 3.3.9, Boost 1.75.0), with the following
error message:
```
In file included from /usr/include/eigen3/Eigen/Core:368,
from ./hpp-fcl-1.7.3/include/hpp/fcl/data_types.h:41,
from ./hpp-fcl-1.7.3/include/hpp/fcl/collision.h:43,
from ./hpp-fcl-1.7.3/test/serialization.cpp:40:
/usr/include/eigen3/Eigen/src/Core/util/ForwardDeclarations.h: In instantiation of 'struct Eigen::internal::accessors_level':
/usr/include/eigen3/Eigen/src/Core/util/ForwardDeclarations.h:109:58: required by substitution of 'template class SPT> void boost::serialization::load(Archive&, SPT&, unsigned int) [with Archive = boost::archive::text_iarchive; SPT = ]'
/usr/include/boost/serialization/split_free.hpp:58:13: required from 'static void boost::serialization::free_loader::invoke(Archive&, T&, unsigned int) [with Archive = boost::archive::text_iarchive; T = Eigen::Map, 0, Eigen::Stride<0, 0> >]'
/usr/include/boost/serialization/split_free.hpp:74:18: required from 'void boost::serialization::split_free(Archive&, T&, unsigned int) [with Archive = boost::archive::text_iarchive; T = Eigen::Map, 0, Eigen::Stride<0, 0> >]'
./hpp-fcl-1.7.3/include/hpp/fcl/serialization/eigen.h:82:17: required from 'void boost::serialization::serialize(Archive&, Eigen::Map&, unsigned int) [with Archive = boost::archive::text_iarchive; PlainObjectBase = Eigen::Matrix; int MapOptions = 0; StrideType = Eigen::Stride<0, 0>]'
/usr/include/boost/serialization/serialization.hpp:109:14: required from 'void boost::serialization::serialize_adl(Archive&, T&, unsigned int) [with Archive = boost::archive::text_iarchive; T = Eigen::Map, 0, Eigen::Stride<0, 0> >]'
/usr/include/boost/archive/detail/iserializer.hpp:187:40: [ skipping 29 instantiation contexts, use -ftemplate-backtrace-limit=0 to disable ]
/usr/include/boost/archive/detail/common_iarchive.hpp:67:22: required from 'void boost::archive::detail::common_iarchive::load_override(T&) [with T = hpp::fcl::BVHModelBase; Archive = boost::archive::text_iarchive]'
/usr/include/boost/archive/basic_text_iarchive.hpp:70:52: required from 'void boost::archive::basic_text_iarchive::load_override(T&) [with T = hpp::fcl::BVHModelBase; Archive = boost::archive::text_iarchive]'
/usr/include/boost/archive/text_iarchive.hpp:82:52: required from 'void boost::archive::text_iarchive_impl::load_override(T&) [with T = hpp::fcl::BVHModelBase; Archive = boost::archive::text_iarchive]'
/usr/include/boost/archive/detail/interface_iarchive.hpp:68:36: required from 'Archive& boost::archive::detail::interface_iarchive::operator>>(T&) [with T = hpp::fcl::BVHModelBase; Archive = boost::archive::text_iarchive]'
./hpp-fcl-1.7.3/test/serialization.cpp:122:10: required from 'void test_serialization(const T&, T&, int) [with T = hpp::fcl::BVHModelBase]'
./hpp-fcl-1.7.3/test/serialization.cpp:231:23: required from here
/usr/include/eigen3/Eigen/src/Core/util/ForwardDeclarations.h:32:48: error: incomplete type 'Eigen::internal::traits' used in nested name specifier
32 | enum { has_direct_access = (traits::Flags & DirectAccessBit) ? 1 : 0,
| ^~~~~
/usr/include/eigen3/Eigen/src/Core/util/ForwardDeclarations.h:33:47: error: incomplete type 'Eigen::internal::traits' used in nested name specifier
33 | has_write_access = (traits::Flags & LvalueBit) ? 1 : 0,
| ^~~~~
```
This bug was already reported at https://stackoverflow.com/questions/54534047/eigen-matrix-boostserialization-c17
and discussed at https://gitlab.com/libeigen/eigen/-/issues/1676
A minimal reproducible example is provided at https://godbolt.org/z/uIy1Uu
This example shows that this bug is triggered only on GCC >= 7 (other
compilers are not affected) and on C++17 (which is the default for
GCC >= 11).
Also, hpp-fcl 1.7.3 compilation is fine on Arch either by using clang or
by explicitely setting GCC on C++14.
This commit implements the workaround provided by Christoph Hertzberg on
the Eigen issue discussion.
Another workaround is already available in Eigen >= 3.3.8:
https://gitlab.com/libeigen/eigen/-/commit/2aa9eb3ce8fa6b2d61dce5be9d6d6460a28080c4
But this doesn't fix the build of hpp-fcl 1.7.3.
Another workaround was proposed to boost::serialization, but was
rejected: boostorg/serialization#144
The main bug in GCC got no attention for the last 2 years:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84075
Previously the forward declaration referred to a non-existing
class U.Note that the
class Uin the template parameter of the template parameterSPTwas not the same as theclass Uin theSPT<class U> &t.This wrong forward declaration causes issues with g++7 and g++8 in c++17 mode: