Set CXX standard to 17 #635

oysstu · 2021-11-23T16:17:03Z

Set CXX standard to 17. Valid for Galactic and later (#634).

sloretz

I'd rather this used target_compile_features(target_name PUBLIC cxx_std_17) instead, but I'd treat that as an enhancement rather than blocking this PR.

LGTM with green CI

sloretz · 2021-11-23T22:49:32Z

CI

repos: https://gist.githubusercontent.com/sloretz/6afda0358f48c644a0d292aae353e449/raw/e2f14016b0815e8c89af6233752f9a1cee1b8320/ros2.repos
build: --packages-up-to rosidl_runtime_c rosidl_runtime_c rosidl_typesupport_introspection_cpp rosidl_typesupport_interface rosidl_runtime_cpp rosidl_generator_cpp
test: --packages-select rosidl_runtime_c rosidl_runtime_c rosidl_typesupport_introspection_cpp rosidl_typesupport_interface rosidl_runtime_cpp rosidl_generator_cpp)

Jobs

Linux
Linux-aarch64
macOS
Windows

aprotyas

Thank you for the PR! This looks good to me on a first pass, I think you missed one instance in rosidl_typesupport_introspection_cpp_generate_interfaces.cmake#L101

if(CMAKE_COMPILER_IS_GNUCXX OR CMAKE_CXX_COMPILER_ID MATCHES "Clang")
  set_target_properties(${rosidl_generate_interfaces_TARGET}${_target_suffix} PROPERTIES
    CXX_STANDARD 14
    COMPILE_OPTIONS -Wall -Wextra -Wpedantic
  )

aprotyas · 2021-11-24T00:12:10Z

The warnings in the MSVC build come from the deprecation of std::wstring_convert and of the <codecvt> header in C++17 - these are used in rosidl_runtime_cpp/traits.hpp.

The deprecation message isn't benign because:

The C++ Standard doesn't provide equivalent non-deprecated functionality;
consider using MultiByteToWideChar() and WideCharToMultiByte() from instead.

The suggestion here is not portable, being part of the Win32 API. The deprecation needs to be dealt with on all platforms regardless.

There's a couple of ways to approach this:

According to https://isocpp.org/files/papers/p0636r0.html, the recommendation is "Users should use dedicated text-processing libraries instead."
- utfcpp is a good library to do so, but that would introduce an external dependency in a core package.
Silence this specific deprecation warning with #define _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING and hope that said classes/header are not removed from the standard library before proper Unicode support is added in C++>23.
- I think at least a subset of the deprecations can still be migrated away from, namely the conversion facet instance std::codecvt_utf8_utf16<char16_t>, since that is mostly syntactic sugar for std::codecvt<char16_t, char, std::mbstate_t>.

oysstu · 2021-11-24T09:19:17Z

I'd rather this used target_compile_features(target_name PUBLIC cxx_std_17) instead, but I'd treat that as an enhancement rather than blocking this PR.

LGTM with green CI

Yes, that seems to be the modern approach to setting the C++ standard. Would also require upping the minimum cmake version to 3.10 in order to have support for MSVC. This is also a change that could be applied to all c++ packages.

Thank you for the PR! This looks good to me on a first pass, I think you missed one instance in
...

Nice catch, I've pushed another commit that changes this one as well.

The warnings in the MSVC build come from the deprecation of std::wstring_convert and of the <codecvt> header in C++17 - these are used in rosidl_runtime_cpp/traits.hpp.
...

According to the deprecation statement, the codecvt "library component should be retired to Annex D, along side , until a suitable replacement is standardized" . I guess this means that it won't be removed for a long time.

aprotyas · 2021-11-24T10:02:46Z

According to the deprecation statement, the codecvt "library component should be retired to Annex D, along side , until a suitable replacement is standardized". I guess this means that it won't be removed for a long time.

In the spirit of not fixing what isn't broken (yet!), I propose this deprecation be silenced with the _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING macro. It looks like @clalancette has a working branch that achieves this, check 0c283d0.

if(MSVC)
  ament_export_definitions(_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)
endif()

oysstu · 2021-11-25T08:26:16Z

Should I merge that branch into this PR?

aprotyas · 2021-11-25T08:40:34Z

Should I merge that branch into this PR?

Sure - you could also cherry-pick 8f855b7 and 0c283d0 onto your working branch. That might be less friction than a merge.

aprotyas

Thanks for iterating! LGTM with green CI.

Rerunning CI from above:

repos: https://gist.githubusercontent.com/sloretz/6afda0358f48c644a0d292aae353e449/raw/e2f14016b0815e8c89af6233752f9a1cee1b8320/ros2.repos
build: --packages-up-to rosidl_runtime_c rosidl_runtime_c rosidl_typesupport_introspection_cpp rosidl_typesupport_interface rosidl_runtime_cpp rosidl_generator_cpp
test: --packages-select rosidl_runtime_c rosidl_runtime_c rosidl_typesupport_introspection_cpp rosidl_typesupport_interface rosidl_runtime_cpp rosidl_generator_cpp

Jobs

Linux
Linux-aarch64
macOS
Windows

aprotyas · 2021-11-25T19:34:09Z

It looks like MSVC throws the deprecation warnings despite 7ec237e. I assumed the definition exporting mechanics would do the job. Any clues? May just have to define the silencing macro in the rosidl_runtime_cpp/traits.hpp header.

oysstu · 2021-11-25T20:40:44Z

It looks like MSVC throws the deprecation warnings despite 7ec237e. I'm not sure how the definition exporting mechanics is supposed to work. Any clues? May just have to define the silencing macro in the rosidl_runtime_cpp/traits.hpp header.

I don't think the 'ament_export_definitions' macro adds the preprocessor definition at all, it seems like it may have something to do with exporting definitions to downstream packages, but I can't find any documentation. For now, I think it's best to simply use add_compile_options (ROS hasn't begun to use the modern target-specific cmake style as far as I know). Not sure how this affects other packages that include this header, I'm not that familiar with the ament_cmake machinery (perhaps this is what ament_export_definitions does).

Defining it in the header isn't a good option, as it needs to be defined before the STL libraries are pulled in (https://devblogs.microsoft.com/cppblog/c17-feature-removals-and-deprecations/).

e.g.

if (MSVC)
    add_compile_options(_SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)
endif()```

aprotyas · 2021-11-26T23:07:36Z

ROS hasn't begun to use the modern target-specific cmake style as far as I know

Every PR can be a small step in that direction! ;-)

Defining it in the header isn't a good option, as it needs to be defined before the STL libraries

Understood. Since add_compile_options appends to the project-scope COMPILE_OPTIONS variable, it might be better to limit the scope of the compile definition to direct consumers as such?

if (MSVC)
  target_compile_definitions(${PROJECT_NAME} INTERFACE
    _SILENCE_CXX17_CODECVT_HEADER_DEPRECATION_WARNING)
endif()

The codecvt library is marked as deprecated in MSVC, but has no replacement in the standard. According to the deprecation notice, the library will not be removed until a suitable replacement has been standardized. In order to avoid pulling in additional third party libraries, the deprecation warning is disabled until the time comes that a replacement can be made (probably C++23) Signed-off-by: Øystein Sture <os@skarvtech.com>

oysstu · 2021-12-02T11:09:31Z

Ok, I've added the proposed change and rebased/squashed the commits. Hopefully the current change successfully disables the warning on MSVC.

aprotyas · 2021-12-02T21:09:12Z

Thanks for iterating @oysstu! There's a linter error in the CMakeLists.txt file, I think replacing if (MSVC) with if(MSVC) should fix it. Can you commit that change and then I'll run CI again?

Signed-off-by: Øystein Sture <os@skarvtech.com>

aprotyas · 2021-12-03T23:22:52Z

Rerunning CI:

repos: https://gist.githubusercontent.com/sloretz/6afda0358f48c644a0d292aae353e449/raw/e2f14016b0815e8c89af6233752f9a1cee1b8320/ros2.repos
build: --packages-up-to rosidl_runtime_c rosidl_runtime_c rosidl_typesupport_introspection_cpp rosidl_typesupport_interface rosidl_runtime_cpp rosidl_generator_cpp
test: --packages-select rosidl_runtime_c rosidl_runtime_c rosidl_typesupport_introspection_cpp rosidl_typesupport_interface rosidl_runtime_cpp rosidl_generator_cpp

Jobs

Linux
Linux-aarch64
macOS
Windows

hidmic · 2021-12-23T17:37:16Z

@aprotyas @clalancette shall we get this moving?

aprotyas · 2021-12-23T18:18:13Z

I think this PR is ready to go, feel free to merge it please!

hidmic · 2022-01-03T15:38:34Z

It's been a while. Running full CI to be on the safe side:

Linux
Linux-aarch64
macOS
Windows

aprotyas · 2022-01-05T14:00:19Z

Test failures look unrelated. The windows test failure is reported as flaky.

hidmic · 2022-01-05T14:41:59Z

Test failures look unrelated. The windows test failure is reported as flaky.

Indeed. Going in. Thanks @oysstu !

oysstu mentioned this pull request Nov 23, 2021

Galactic: set CMAKE_CXX_STANDARD to 17 #634

Closed

sloretz approved these changes Nov 23, 2021

View reviewed changes

aprotyas requested changes Nov 23, 2021

View reviewed changes

oysstu requested a review from aprotyas November 25, 2021 09:28

aprotyas approved these changes Nov 25, 2021

View reviewed changes

Resolve linter error

e3dc727

Signed-off-by: Øystein Sture <os@skarvtech.com>

hidmic merged commit 2ec0023 into ros2:master Jan 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set CXX standard to 17 #635

Set CXX standard to 17 #635

oysstu commented Nov 23, 2021

sloretz left a comment

sloretz commented Nov 23, 2021

aprotyas left a comment

aprotyas commented Nov 24, 2021 •

edited

Loading

oysstu commented Nov 24, 2021 •

edited

Loading

aprotyas commented Nov 24, 2021 •

edited

Loading

oysstu commented Nov 25, 2021

aprotyas commented Nov 25, 2021

aprotyas left a comment •

edited

Loading

aprotyas commented Nov 25, 2021 •

edited

Loading

oysstu commented Nov 25, 2021

aprotyas commented Nov 26, 2021 •

edited

Loading

oysstu commented Dec 2, 2021

aprotyas commented Dec 2, 2021

aprotyas commented Dec 3, 2021

hidmic commented Dec 23, 2021

aprotyas commented Dec 23, 2021

hidmic commented Jan 3, 2022

aprotyas commented Jan 5, 2022

hidmic commented Jan 5, 2022

Set CXX standard to 17 #635

Set CXX standard to 17 #635

Conversation

oysstu commented Nov 23, 2021

sloretz left a comment

Choose a reason for hiding this comment

sloretz commented Nov 23, 2021

aprotyas left a comment

Choose a reason for hiding this comment

aprotyas commented Nov 24, 2021 • edited Loading

oysstu commented Nov 24, 2021 • edited Loading

aprotyas commented Nov 24, 2021 • edited Loading

oysstu commented Nov 25, 2021

aprotyas commented Nov 25, 2021

aprotyas left a comment • edited Loading

Choose a reason for hiding this comment

aprotyas commented Nov 25, 2021 • edited Loading

oysstu commented Nov 25, 2021

aprotyas commented Nov 26, 2021 • edited Loading

oysstu commented Dec 2, 2021

aprotyas commented Dec 2, 2021

aprotyas commented Dec 3, 2021

hidmic commented Dec 23, 2021

aprotyas commented Dec 23, 2021

hidmic commented Jan 3, 2022

aprotyas commented Jan 5, 2022

hidmic commented Jan 5, 2022

aprotyas commented Nov 24, 2021 •

edited

Loading

oysstu commented Nov 24, 2021 •

edited

Loading

aprotyas commented Nov 24, 2021 •

edited

Loading

aprotyas left a comment •

edited

Loading

aprotyas commented Nov 25, 2021 •

edited

Loading

aprotyas commented Nov 26, 2021 •

edited

Loading