Removed the explicit '-std=c++14' compile flag; set the CMake STANDAR…

[svwilliams]

Addendum to PR #112

• Removed the explicit '-std=c++14' compile flag
• Set the CMake STANDARD_REQUIRED flag

[efernandez]

@svwilliams I think you could have reverted 9933456 as I do in #120 and then do the rest of the changes. As I mention in the description of that PR there are still two instances of target_compile_features:

fuse_constraints/CMakeLists.txt
219:  target_compile_features(test_marginal_constraint PRIVATE cxx_generic_lambdas)
237:  target_compile_features(test_marginalize_variables PRIVATE cxx_generic_lambdas)

[efernandez]

BTW I'm still not sure that not using target_compile_features is a better approach. It allows to specify the specific C++ features required, that could be in different standards, and it even allows to select the whole set of features supported by one standard: https://cmake.org/cmake/help/v3.14/manual/cmake-compile-features.7.html

That should allow the building infrastructure to easily switch to a newer version of the standard w/o making any changes to the source code. For example, if we ever switch to C++17, do we have to update the CMakeLists.txt files to update all the set(CMAKE_CXX_STANDARD 14)?

I'm also afraid that set(CMAKE_CXX_STANDARD 14) could pollute the compilation of other modules in the same workspace, but I'm not 100% sure of this one. This might indeed propagate into fuse dependencies. If their headers require a standard newer than C++14, I think this should be changed for things to build because the minimum standard required and exported by the dependencies would be preceded by the value of CMAKE_CXX_STANDARD set here.

[svwilliams]

While I like the theory behind specifying the features you need and letting the build system select the correct compile flags, in practice I found the system to be cumbersome and incomplete. Here's my rant:

First, not all C++11/C++14 standard features have a corresponding CMake target_compile_feature flag. For example, if you only needed to use shared pointers, there is no compile feature you can specify. It seems to me there are use cases where the available CMake flags are insufficient to select the required compiler settings. This is not necessarily an issue with fuse, as it uses many C++11/C++14 features, but I do find the selected granularity of the system strange. I end up looking at the list of CMake feature flags and then scanning the code looking for usage of the feature, rather than the other way around.

The fuse code uses many C++11/C++14 features. To be a good and proper code maintainer, you need to specify each one, and determine if it is a public or private build requirement. Since this is done per-target, the list must be generated for each library, executable and test target separately. Having to track each standard feature that is used increases the maintenance burden quite a bit. And since this list is defined at the CMake level and not the compiler level, it is nearly impossible to tell if you have correctly captured all of the used features -- the compiler will not warn you if you are missing something. The odds of this list being maintained during refactors is slim. For reference, here is the branch where I was trying to get the per-feature list working before I decided the whole thing was too much trouble. I never got it quite working correctly, and I didn't even attempt to get the unit tests configured.

The benefit of tracking each feature is marginal. In practice, fuse will only be compiled using gcc version 5.3 or later (Xenial/Kinetic or later). This means all of this work tracking individual language features is largely wasted, since only features supported by gcc 5.3 or later are used inside of fuse, and non-gcc compilers are definitely not tested, and will probably never be used. In the end, all of this extra work is roughly equivalent to just setting the -std=c++14 compile flag.

I do take the "polluting the CMake settings for other projects" bit seriously, but I'm not sure what the solution is. If I could use the CMake cxx_std_14 setting for target_compile_feature command, then the minimal additional effort of specifying the C++ standard for each build target would be worth it. However, the language standard settings were not introduced into CMake until version 3.8 and the default Xenial CMake version is 3.5. So those shortcut settings cannot be used without dropping support for Xenial/Kinetic.

My understanding of CXX_STANDARD_REQUIRED is that (a) unless that is set, the compiler is allowed to fallback to a previous standard, which we do not want, and (b) that the compiler is allowed to select a newer version than what is listed. So if something else requires C++17, then everything is allowed to move to C++17.

What I could do is set the CXX_STANDARD and CXX_STANDARD_REQUIRED per-target, like so:

set_target_properties(${PROJECT_NAME}
  PROPERTIES
    CXX_STANDARD 14
    CXX_STANDARD_REQUIRED YES
)

This avoids setting the global CMAKE_CXX_STANDARD variable. That should prevent pollution of other projects in the same CMake workspace, and only requires minimal extra effort.

Regarding the two existing instances of target_compile_features:

fuse_constraints/CMakeLists.txt
219:  target_compile_features(test_marginal_constraint PRIVATE cxx_generic_lambdas)
237:  target_compile_features(test_marginalize_variables PRIVATE cxx_generic_lambdas)

I'm assuming these are not needed if we are setting the target C++ standard to C++14 explicitly. I believe those were removed in the branch you tested, and Ceres and fuse still compiled correctly?

[efernandez]

@svwilliams I think you're right when you say things would be hard to maintain if you use target_compile_feature.

Regarding set(CMAKE_CXX_STANDARD 14), I think it's only this that could pollute the build process of other projects. 👍 for switching to set_target_properties, although I'm not sure if there's a problem with add_compile_options(-std=c++14 ...) 🤔

Finally, the two existing instances of target_compile_features should be removed after all the other changes. Everything builds fine for me w/o them. That's what I tested in #120

[gavanderhoorn]

Suggestion: for CMake >=3.8, you could use the meta compile features (top 4 of this list).

To force C++14 on a target: target_compile_features(<TARGET> ... cxx_std_14). This does not need add a list that needs to be maintained.

Advantage of doing it this way is that if/when targets are properly exported, consumers will automatically require these compile features as well (CMake takes care of transitively managing these).

CMAKE_CXX_STANDARD et al. do not get exported to consumers, which requires exporting this requirement in a different (ie: typically manual) way.

---

Edit: if Kinetic+Xenial should also be supported, the following trick could perhaps be used to spec C++14 for both CMake <3.8 and >= 3.8:

target_compile_features(<TARGET> ...
  # on CMake 3.8+ use meta-feature
  $<$<NOT:$<VERSION_LESS:$CMAKE_VERSION,3.8>>:cxx_std_14>
  # the following should get us C++14 on older versions of CMake
  $<$<VERSION_LESS:$CMAKE_VERSION,3.8>:cxx_decltype_auto>)

[svwilliams]

What I should do is drop support for Xenial/Kinetic since Xenial has reached end of life, but Xenial has a strangely long life in the robotics community. I will switch over to the cxx_std_14 flag as soon as I feel like I can make Bionic/Melodic the default target platform.

[gavanderhoorn]

C++14 is not the problem. That's supported on Xenial.

It's CMake that is a bit old (on Xenial).

But with the generator expression I showed in my previous comment everything should work on all CMake versions (on Xenial and newer).

[svwilliams]

Well, yes and no. It is abusing the design intent of the named-feature target_compile_features command. You are relying on the fact that requesting cxx_decltype_auto will translate into the correct compile flag all the time...which it probably will in practice. But my desire to maintain conditional complication for various CMake/gcc versions is very small.

[gavanderhoorn]

You are relying on the fact that requesting cxx_decltype_auto will translate into the correct compile flag all the time...which it probably will in practice.

Seeing as N3638 was only accepted into C++14 and is on GCC, Clang, Intel and MSVC only supported in versions that actually support C++14, it would seem like an acceptable work-around to having to list each and every feature.

But it's all up to you.

I just wanted to make you aware of something that seems to tick all the boxes without requiring anything special or manual maintenance.

[svwilliams]

Yep. Definitely appreciated. Keep the suggestions coming. At this time I'm not personally comfortable maintaining complicated CMake expressions, but it is nice to have a solution ready if this becomes issue for users.