Compilation error with boost::placeholders

[ToniRV]

Description

Using boost::placeholders in 16.04 is not supported (out of the box). Is there a reason why you use boost:: and not std:: ?

/root/gtsam/gtsam/inference/Symbol.cpp:66:61: error: 'boost::placeholders' has not been declared
   return boost::bind(&Symbol::chr, boost::bind(make, boost::placeholders::_1)) == c;

Steps to reproduce

1. Compile in fresh 16.04 (docker ideally)

Environment

Ubuntu 16.04

[ToniRV]

I think the offending commit is: https://github.com/borglab/gtsam/commit/5a2ff198f0b8d5a7200354d900ce04713ea6aacb

[ProfFan]

796 is the related PR. I think a workaround like https://forum.freecadweb.org/viewtopic.php?t=47636 is enough to fix this.

[varunagrawal]

Huh looks like std::placeholders is supported in C++11. Let's update it!

[varunagrawal]

@acxz tagging for visibility.

[varunagrawal]

@ToniRV FYI the minimum version of Boost we support is now 1.67 due to issues with serialization in prior versions (there is some issue in this repo detailing this). Ubuntu 16.04 comes with 1.58, so it is not officially supported. The best course of action would be for you to install Boost 1.67 or above. Closing since this is not a GTSAM issue (per se).

[ToniRV]

Ok, thanks. I'd still strongly suggest using std:: for everything. Is there a reason why you use boost::bind or boost::placeholders?

I agree for serialization it is needed (we do use it as well in Kimera), but for binding and placeholding? I don't think it is necessary, no? I'm curious to know if I'm missing a bug I'm unaware of?

[ProfFan]

I don't think there is any except maintaining API stability. boost::bind does have many differences against std::bind, but I think they do not matter in our use case (need confirm).

[varunagrawal]

@ToniRV I agree with you. I actually tried doing a global replace of bind and placeholders from boost to std, but I ran into some compilation errors which didn't make sense to me. My guess is that the functional returned by boost and that returned by std have some differences so this would be more effort than I have the cycles for. Can you create an issue for this? I'd like to get to it some day, just not now. :)

[berndpfrommer]

Is this PR what's breaking the xenial build on the ubuntu packaging server (note to @jlblancoc)?

[ 36%] Building CXX object gtsam/CMakeFiles/gtsam.dir/geometry/EssentialMatrix.cpp.o
cd /<<PKGBUILDDIR>>/obj-aarch64-linux-gnu/gtsam && /usr/bin/aarch64-linux-gnu-g++   -DNDEBUG -Dgtsam_EXPORTS -I/<<PKGBUILDDIR>>/gtsam/3rdparty/metis/include -I/<<PKGBUILDDIR>>/gtsam/3rdparty/metis/libmetis -I/<<PKGBUILDDIR>>/gtsam/3rdparty/metis/GKlib -isystem /<<PKGBUILDDIR>>/gtsam/3rdparty/SuiteSparse_config -isystem /<<PKGBUILDDIR>>/gtsam/3rdparty/Spectra -isystem /<<PKGBUILDDIR>>/gtsam/3rdparty/CCOLAMD/Include -I/<<PKGBUILDDIR>> -I/<<PKGBUILDDIR>>/obj-aarch64-linux-gnu -I/<<PKGBUILDDIR>>/CppUnitLite -I/usr/include/eigen3  -g -O2 -fPIE -fstack-protector-strong -Wformat -Werror=format-security -Wdate-time -D_FORTIFY_SOURCE=2  -O2 -g -DNDEBUG -fPIC   -std=c++11 -Wall -fPIC -Wreturn-local-addr -Werror=return-local-addr -Wreturn-type -Werror=return-type -Wformat -Werror=format-security -Wsuggest-override -g -O3 -Wno-unused-local-typedefs -o CMakeFiles/gtsam.dir/geometry/EssentialMatrix.cpp.o -c /<<PKGBUILDDIR>>/gtsam/geometry/EssentialMatrix.cpp
In file included from /<<PKGBUILDDIR>>/gtsam/nonlinear/Values.h:542:0,
                 from /<<PKGBUILDDIR>>/gtsam/nonlinear/NonlinearFactor.h:23,
                 from /<<PKGBUILDDIR>>/gtsam/slam/TriangulationFactor.h:18,
                 from /<<PKGBUILDDIR>>/gtsam/geometry/triangulation.h:26,
                 from /<<PKGBUILDDIR>>/gtsam/geometry/triangulation.cpp:19:
/<<PKGBUILDDIR>>/gtsam/nonlinear/Values-inl.h: In member function ‘gtsam::Values::Filtered<ValueType> gtsam::Values::filter(const boost::function<bool(long unsigned int)>&)’:
/<<PKGBUILDDIR>>/gtsam/nonlinear/Values-inl.h:248:7: error: ‘placeholders’ is not a member of ‘boost’
       boost::placeholders::_1), *this);
       ^
/<<PKGBUILDDIR>>/gtsam/nonlinear/Values-inl.h:248:7: note: suggested alternatives:
In file included from /usr/include/eigen3/Eigen/Core:225:0,
                 from /usr/include/eigen3/Eigen/Dense:1,
                 from /<<PKGBUILDDIR>>/gtsam/base/OptionalJacobian.h:22,
                 from /<<PKGBUILDDIR>>/gtsam/base/Matrix.h:27,
                 from /<<PKGBUILDDIR>>/gtsam/base/Manifold.h:22,
                 from /<<PKGBUILDDIR>>/gtsam/base/Lie.h:25,
                 from /<<PKGBUILDDIR>>/gtsam/base/VectorSpace.h:11,
                 from /<<PKGBUILDDIR>>/gtsam/geometry/Point2.h:20,
                 from /<<PKGBUILDDIR>>/gtsam/geometry/Cal3.h:24,
                 from /<<PKGBUILDDIR>>/gtsam/geometry/Cal3_S2.h:24,
                 from /<<PKGBUILDDIR>>/gtsam/geometry/triangulation.h:21,
                 from /<<PKGBUILDDIR>>/gtsam/geometry/triangulation.cpp:19:
/usr/include/c++/5/functional:776:3: note:   ‘std::placeholders’

[varunagrawal]

Yeah. Xenial has an older version of boost which has this issue as well as some serialization bugs. Would it be recommended to revert the PR or somehow get Boost 1.67 as the correct dependency?

[varunagrawal]

I'm reopening this since I found the main differences for bind between boost and std, and will be making a PR this weekend.

https://stackoverflow.com/questions/10555566/difference-between-c11-stdbind-and-boostbind

[varunagrawal]

@ToniRV @berndpfrommer can we check if the latest develop works for your respective use cases? Please do let me know so I can make the required fixes. :slightly_smiling_face:

[ToniRV]

It works, thank you!

[berndpfrommer]

Nightly build succeeded.

[varunagrawal]

Yayy! The only unfortunate thing is that std::optional is C++17, else we would have come very close to potentially eliminating boost as a dependency.

[ToniRV]

That would have been great indeed! I did manage to replace boost::optional by just using pointer semantics:

• Optional input? const T* foo
• Optional output? T* bar

And then the code is the same:

• if(foo) compute(*foo);
• if(bar) *bar = T();

But boost::optional makes things explicit... despite the extra verbosity.

[ProfFan]

boost::optional is much more powerful than just a pointer, for example pointers do not carry lifetime information. In general raw pointers should be avoided unless absolutely necessary.

I think a good alternative is a simple header that implements optional in pure C++11, with a suitable license so that we can embed into GTSAM.

[ToniRV]

:smile: so in my experience, the discussion btw boost::optional or pointers can last for a long long time without a clear winner... It's up to you, but here are my two cents:

• Pros for using pointers are: -- no external dependency (unless c++17) -- less verbose

• Cons for using pointers are: -- Old fashioned unexplicit code, if you don't tell your developers they won't necessarily know what is going on.

• Pros for using optional: -- can't be more explicit than that

• Cons for using optional: -- copy-ellision is not obvious -- verbose -- extra-dependency (until c++17)

If we were using c++17 I'd go with std::optional probably.

[ToniRV]

Btw @ProfFan, raw pointers are totally fine when they are not mindlessly heap-allocated with new, and they are part of the google c++ code-style for input/output and output from functions.

[varunagrawal]

I managed to find (via StackOverflow) a repo for C++11 compatible optional. We can always alias tl::optional to gtsam::optional and later update the alias to std::optional if and when we decide to be C++17 compatible.

[ProfFan]

I second tl, it's the one with best standard conformity among all the choices: https://github.com/martinmoene/optional-lite/issues/61

[dellaert]

All, Why are we getting all excited here ? There are more dependencies on boost, so unless this is the last boost dependency I am not keen on changing this or bringing in tl...

[berndpfrommer]

Not sure what happened, but the builds on Ubuntu 16.04 (xenial) are failing with this error message:

-- Could NOT find Boost: Found unsuitable version "1.58.0", but required is at least "1.65" (found /usr/include) The related commit is from Aug 20th (my bad I didn't raise this issue earlier):

https://github.com/borglab/gtsam/commit/42b75253373bae499b40ac34631b8895c195a0f4

Have we officially dropped support for Ubuntu 16.04? Not a problem, I can take it off the build list, just need to know.

[ProfFan]

I think we just require a newer boost because of the boost::serialization bug that whose fix is not backported to 1.58.x.

I think it’s fair to drop it from builds since 16.04 is EOL.

[dellaert]

I agree with that, but I remember we had this discussion earlier - @varunagrawal?