Bump to c++17

[rotu]

This allows us to use std::filesystem, as the logic in filesystem_helper sniffs the current c++ standard. CMake should fall back gracefully to c++14 if c++17 is not available.

[nuclearsandwich]

Thanks for the contribution. However, with ros-infrastructure/rep@953a64d C++14 is still going to be the target C++ standard for Foxy. So this PR will have to wait. The community discussion which prompted the change starts with ros-infrastructure/rep#217 (comment)

[rotu]

@nuclearsandwich I know we're targeting C++14 but I still believe this PR was correct as written.

even if we're targeting CMake 14, setting CMAKE_CXX_STANDARD to a newer version will fall back to the previous supported standard if 17 is unavailable.

<experimental/filesystem> is not part of a C++ standard version, and suppressing the warning is not a long-term solution. In the future, both C++14 (without filesystem) and C++17 (with filesystem) will work, but <experimental/filesystem>will break.

The conditional here if ... __cplusplus >= 201703L ... will never be true if we restrict to an earlier c++14 https://github.com/ros/pluginlib/blob/ros2/pluginlib/include/pluginlib/impl/filesystem_helper.hpp#L40.

[nuclearsandwich]

Thanks for the added context. I'd re-open the PR now but GitHub is preventing me from doing so since the branch has been "force-pushed or recreated".

even if we're targeting CMake 14, setting CMAKE_CXX_STANDARD to a newer version will fall back to the previous supported standard if 17 is unavailable.

Thanks for the info and doc link. I didn't know about the decay behavior in CMAKE_CXX_STANDARD. That certainly makes this discussion worth re-opening.

The conditional here if ... __cplusplus >= 201703L ... elif ... handles the case of pre-C++17: /pluginlib/include/pluginlib/impl/filesystem_helper.hpp@ros2#L40

Acknowleged, that handles the case we know to handle. Which brings me to my two main concerns with the standard bump which may or may not be well-founded:

1.Bumping the standard for this opens up the possibility of unintentionally adding more C++17-only features without noticing right away as none of our tier one platform compilers lack C++17 support.
2. This doesn't just bump the standard for pluginlib but also for any package that's using it, which widens the regression surface for that first point.

suppressing the warning is not a long-term solution.

I don't disagree. I tried a few things locally before settling for suppressing the warning. Clang 10 doesn't provide a clean way to use either experimental/filesystem or std::filesystem from C++14 without suppressing the warning.

I'm not sure whether I like the idea of only setting CMAKE_CXX_STANDARD to 17 when Clang>=9 is detected but it does at least ensure that the other platforms are testing the C++14 path.

[rotu]

I'd re-open the PR now but

I can open a new PR.

This doesn't just bump the standard for pluginlib but also for any package that's using it, which widens the regression surface for that first point.

Agreed. It's a consequence of trying to implement it in the header, which means you impose additional restrictions on downstream packages. pluginlib-extras.cmake I can try to fix this in the PR The answer is to move it into a library instead of a header.

Clang 10 doesn't provide a clean way to use either experimental/filesystem or std::filesystem from C++14 without suppressing the warning.

Please meditate on this! Clang is in the right:

• experimental/filesystem is not part of either C++14 nor C++17, and in a compiler that implements C++17 should not be used.
• std::filesystem is not part of C++14.