Drop wrong template specialization #1926

jspricke · 2022-05-10T21:20:44Z

This fails with g++ -std=gnu++20.

jspricke · 2022-05-10T21:22:27Z

minimal example: https://paste.debian.net/1240487/

griswaldbrooks

Interesting. I'm not sure why @wjwwood felt the need to template this

jspricke · 2022-05-11T06:36:16Z

Not sure why TestNodeGraph.construct_from_node fails, that seems unrelated to this change. Can you retest this?

fujitatomoya · 2022-05-11T17:50:10Z

Rpr job seems to be unrelated. I will run CI to verify.

fujitatomoya · 2022-05-11T17:53:02Z

CI:

Linux
Linux-aarch64
Windows

wjwwood

lgtm

Was probably a typo that was previously allowed by the compiler.

Why does it pass on this one?

rclcpp/rclcpp/include/rclcpp/subscription_options.hpp

Line 100 in 491475f

SubscriptionOptionsWithAllocator<Allocator>() {}

wjwwood · 2022-05-11T23:52:54Z

Also, generally it is helpful to paste the compiler output of the failure. I had to go to compiler explorer to try and understand what the root cause was.

griswaldbrooks · 2022-05-12T00:12:30Z

I would think that it would be sufficient to paste compiler explorer links moving forward?

griswaldbrooks · 2022-05-12T00:13:26Z

@wjwwood is your buildfarm building against C++20?

wjwwood · 2022-05-12T01:09:54Z

I would think that it would be sufficient to paste compiler explorer links moving forward?

That's fine, but honestly if you have your local stdout from the compiler that's usually enough to go on.

@wjwwood is your buildfarm building against C++20?

We use the standard compiler that comes with the target platforms. In CMake we request the minimum C++ version for the ROS distribution, e.g.:

rclcpp/rclcpp/CMakeLists.txt

Lines 25 to 28 in 491475f

    
           # Default to C++17 
        
           if(NOT CMAKE_CXX_STANDARD) 
        
             set(CMAKE_CXX_STANDARD 17) 
        
           endif()

Both questions depend on the target platform and the ROS distribution, which are defined in REP-2000, e.g. for Humble:

https://www.ros.org/reps/rep-2000.html#humble-hawksbill-may-2022-may-2027

If you assume Ubuntu, we're using 22.04, and require C++17 only. So we're not requesting C++20, at least on Linux. However, other platforms and compilers may up the standard above that, but we don't explicitly test that.

griswaldbrooks · 2022-05-12T01:15:08Z

So then that probably answer's your question
#1926 (review)
Maybe it passes for that one because you're only testing C++17, which makes sense

wjwwood · 2022-05-12T17:27:19Z

Maybe it passes for that one because you're only testing C++17, which makes sense

The code before this pr's change also passed, so yeah I expected it to work with C++17 (our CI).

I was more asking why this pr isn't also changing that line? I assume this pr was created because it failed for one of you two?

griswaldbrooks · 2022-05-12T17:45:33Z

I was more asking why this pr isn't also changing that line? I assume this pr was created because it failed for one of you two?

ah, yes, correct. Technically it was from one of our coworkers that was porting code that I wrote and @jspricke solved

wjwwood · 2022-05-12T18:03:02Z

Right, that's ok, but I guess what I'm asking is if this pr should include a change for subscription options too?

Or failing that, why does that not fail in your system (using -std=gnu++20)?

I'm hesitant to merge this until we fix the other instances too, or explain them.

This fails with g++ -std=gnu++20. Signed-off-by: Jochen Sprickerhof <git@jochen.sprickerhof.de>

jspricke · 2022-05-12T18:33:18Z

Why does it pass on this one?

rclcpp/rclcpp/include/rclcpp/subscription_options.hpp

Line 100 in 491475f

SubscriptionOptionsWithAllocator<Allocator>() {}

Good point, I simply did not test that file. Fixed as well now.

We use the standard compiler that comes with the target platforms. In CMake we request the minimum C++ version for the ROS distribution, e.g.:

rclcpp/rclcpp/CMakeLists.txt

Lines 25 to 28 in 491475f

# Default to C++17

if(NOT CMAKE_CXX_STANDARD)

set(CMAKE_CXX_STANDARD 17)

endif()

Note that this selects exactly the version not the minimum version. I would recommend not to do it as it will bite you in future if other parts want to use newer versions.

wjwwood · 2022-05-12T21:18:47Z

Note that this selects exactly the version not the minimum version. I would recommend not to do it as it will bite you in future if other parts want to use newer versions.

If you don't do that then you get C++14 on some compilers. Also it prevents the user from trying to use a compiler which does not support C++17. You can override it due to the "if not set" logic.

The "other parts", as you put it (I took it to mean downstream packages), can still use newer C++ standards by setting their own value for CMAKE_CXX_STANDARD, with the caveat that we only test and support C++17 in our code, so you might run into things like this pr is fixing, and that what ever they set it to needs to be ABI compatible with C++17, or else they would need to rebuild all of ROS with the CMAKE_CXX_STANDARD set externally for all the packages to the newer value.

So, I don't see any problem with setting the CMAKE_CXX_STANDARD to 17. Did I miss something?

wjwwood · 2022-05-12T21:23:19Z

Another CI:

Linux
Linux-aarch64
Windows

jspricke · 2022-05-12T21:28:44Z

So, I don't see any problem with setting the CMAKE_CXX_STANDARD to 17. Did I miss something?

It depends on how you include this project into other projects and where you set what standard. The better way to do it is to use https://cmake.org/cmake/help/latest/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html

But that could be quiet some work to retrofit.

Btw. Do you plan to drop it now that Rolling is on Jammy with GCC 11 and C++17 as default?

wjwwood · 2022-05-12T21:48:56Z

It depends on how you include this project into other projects and where you set what standard.

Do you have a situation in mind? I can't think of a situation where you're building rclcpp in which you could not override this to a higher C++ standard.

Also, according to the CMake docs, it isn't a requirement so much as a request:

Default value for CXX_STANDARD target property if set when a target is created.

https://cmake.org/cmake/help/latest/variable/CMAKE_CXX_STANDARD.html?highlight=cmake_cxx_standard

The C++ standard whose features are requested to build this target.
...
a compiler which does not support -std=gnu++11 or an equivalent flag will not result in an error or warning, but will instead add the -std=gnu++98 flag if supported. This "decay" behavior may be controlled with the CXX_STANDARD_REQUIRED target property.

https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html#prop_tgt:CXX_STANDARD

So I don't think we're forcing it, as you could with CXX_STANDARD_REQUIRED.

The better way to do it is to use https://cmake.org/cmake/help/latest/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html

We could use cxx_std_17 with target_compile_features(), and I think we discussed doing that at some point, but we never did. I can't remember if there was a reason.

@jspricke Are you basing this recommendation off of some guidelines some where?

Btw. Do you plan to drop it now that Rolling is on Jammy with GCC 11 and C++17 as default?

No, I don't think so, because we need to assert that the compiler being used supports C++17 (in lieu of specifically depending on each feature of C++ that we use). We build ROS 2 on a lot more platforms than Jammy, and in fact we will still have some users building it from source on 20.04, not to mention users building from source on other kinds of platforms/compilers, e.g. QNX's qcc. So it's important to declare what we require in a portable way, rather than assume the underlying compiler supports C++17's feature set.

@nuclearsandwich @clalancette FYI

jspricke · 2022-05-12T22:07:24Z

* William Woodall ***@***.***> [2022-05-12 14:49]:

> It depends on how you include this project into other projects and where you set what standard. Do you have a situation in mind? I can't think of a situation where you're building rclcpp in which you could not override this to a higher C++ standard.

This is open source so you can always work around it, sure :).

Also, according to the CMake docs, it isn't a requirement so much as a request: > Default value for [CXX_STANDARD](https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html#prop_tgt:CXX_STANDARD) target property if set when a target is created. https://cmake.org/cmake/help/latest/variable/CMAKE_CXX_STANDARD.html?highlight=cmake_cxx_standard > The C++ standard whose features are *requested* to build this target. > ... > a compiler which does not support -std=gnu++11 or an equivalent flag will not result in an error or warning, but will instead add the -std=gnu++98 flag if supported. This "decay" behavior may be controlled with the [CXX_STANDARD_REQUIRED](https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD_REQUIRED.html#prop_tgt:CXX_STANDARD_REQUIRED) target property. https://cmake.org/cmake/help/latest/prop_tgt/CXX_STANDARD.html#prop_tgt:CXX_STANDARD So I don't think we're forcing it, as you could with `CXX_STANDARD_REQUIRED`.

I've just tested a minimal project with: project(foo) if(NOT CMAKE_CXX_STANDARD) set(CMAKE_CXX_STANDARD 11) endif() add_executable(foo foo.cpp) and gcc 11 (defaults to C++17) and cmake adds -std=gnu++11 to the compiler flags which forces gcc to C++11. ***@***.*** Are you basing this recommendation off of some guidelines some where? I don't have guidelines at hand, just general rule to use system defaults and not enforce versions. Also I just went through a lot of ROS 1 code to get the C++11 enforcement dropped ;). Anyway, this starts to get off topic for this PR and dropping CMAKE_CXX_STANDARD is not really urgent, so feel free to merge and postpone the discussion.

wjwwood · 2022-05-13T00:10:21Z

Do you have a situation in mind?

This is open source so you can always work around it, sure :).

So, no? I'm just trying to understand why it's a problem, because if it is, I'd like to pursue fixing it.

It's written into the CMake in such a way as to encourage overriding it, so I was asking for a situation where you'd need to edit the source before building it.

I've just tested a minimal project with:

Unfortunately, that's also how target_compile_features() and https://cmake.org/cmake/help/latest/prop_gbl/CMAKE_CXX_KNOWN_FEATURES.html work (they will reduce the C++ standard to what is requested if the default is something newer), but fortunately they respect the PRIVATE/PUBLIC/INTERFACE pattern, so they can be exported downstream (not currently the case).

I made a little example too, which shows that despite that, if one of your dependencies exports something like target_compile_features(${PROJECT_NAME} PUBLIC cxx_std_17), but you use target_compile_features(${PROJECT_NAME} PUBLIC cxx_std_11) on your target, it will upgrade to C++17 anyway (overriding what you used on your target), which might be a problem if you're using something that has been removed since.

Example

Files and output:

% cat src/pkg_a/CMakeLists.txt
cmake_minimum_required(VERSION 3.22)
project(pkg_a)

add_library(${PROJECT_NAME} SHARED foo.cpp)
target_compile_features(${PROJECT_NAME} PUBLIC cxx_std_17)

install(TARGETS ${PROJECT_NAME} EXPORT ${PROJECT_NAME})
install(EXPORT ${PROJECT_NAME}
  FILE pkg_aTargets.cmake
  NAMESPACE ${PROJECT_NAME}::
  DESTINATION lib/cmake/${PROJECT_NAME}
)
install(FILES "pkg_aConfig.cmake" DESTINATION lib/cmake/${PROJECT_NAME})

% cat src/pkg_b/CMakeLists.txt
cmake_minimum_required(VERSION 3.22)
project(pkg_b)

find_package(pkg_a REQUIRED)

add_executable(${PROJECT_NAME} main.cpp)
target_compile_features(${PROJECT_NAME} PUBLIC cxx_std_11)
target_link_libraries(${PROJECT_NAME} PRIVATE pkg_a::pkg_a)

install(TARGETS ${PROJECT_NAME})

% VERBOSE=1 colcon build --event-handlers console_direct+ 2>1 | grep '\-std='
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -Dpkg_a_EXPORTS  -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -mmacosx-version-min=10.14 -fPIC -std=gnu++17 -MD -MT CMakeFiles/pkg_a.dir/foo.cpp.o -MF CMakeFiles/pkg_a.dir/foo.cpp.o.d -o CMakeFiles/pkg_a.dir/foo.cpp.o -c /tmp/test_cmake/src/pkg_a/foo.cpp
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++   -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -mmacosx-version-min=10.14 -std=gnu++17 -MD -MT CMakeFiles/pkg_b.dir/main.cpp.o -MF CMakeFiles/pkg_b.dir/main.cpp.o.d -o CMakeFiles/pkg_b.dir/main.cpp.o -c /tmp/test_cmake/src/pkg_b/main.cpp

The other direction, where your dependency uses cxx_std_11, but you use cxx_std_17, results in the dependency being built with C++11 and your target using C++17.

Example

Files and output:

% cat src/pkg_a/CMakeLists.txt
cmake_minimum_required(VERSION 3.22)
project(pkg_a)

add_library(${PROJECT_NAME} SHARED foo.cpp)
target_compile_features(${PROJECT_NAME} PUBLIC cxx_std_11)

install(TARGETS ${PROJECT_NAME} EXPORT ${PROJECT_NAME})
install(EXPORT ${PROJECT_NAME}
  FILE pkg_aTargets.cmake
  NAMESPACE ${PROJECT_NAME}::
  DESTINATION lib/cmake/${PROJECT_NAME}
)
install(FILES "pkg_aConfig.cmake" DESTINATION lib/cmake/${PROJECT_NAME})

% cat src/pkg_b/CMakeLists.txt
cmake_minimum_required(VERSION 3.22)
project(pkg_b)

find_package(pkg_a REQUIRED)

add_executable(${PROJECT_NAME} main.cpp)
target_compile_features(${PROJECT_NAME} PUBLIC cxx_std_17)
target_link_libraries(${PROJECT_NAME} PRIVATE pkg_a::pkg_a)

install(TARGETS ${PROJECT_NAME})

% VERBOSE=1 colcon build --event-handlers console_direct+ 2>1 | grep '\-std='
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -Dpkg_a_EXPORTS  -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -mmacosx-version-min=10.14 -fPIC -std=gnu++11 -MD -MT CMakeFiles/pkg_a.dir/foo.cpp.o -MF CMakeFiles/pkg_a.dir/foo.cpp.o.d -o CMakeFiles/pkg_a.dir/foo.cpp.o -c /tmp/test_cmake/src/pkg_a/foo.cpp
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++   -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -mmacosx-version-min=10.14 -std=gnu++17 -MD -MT CMakeFiles/pkg_b.dir/main.cpp.o -MF CMakeFiles/pkg_b.dir/main.cpp.o.d -o CMakeFiles/pkg_b.dir/main.cpp.o -c /tmp/test_cmake/src/pkg_b/main.cpp

So I don't see the failure mode.

If the second project (the dependent one) doesn't declare what they use c++ standard wise, then yes they will inherit the requirement from their dependency, which I would say is an issue with that project being under-defined.

Also I just went through a lot of ROS 1 code to get the C++11 enforcement dropped ;).

Why did you have to do that? Was there a build failure or did you just assume that it was out of date and therefore should be removed? (not an unreasonable assumption)

Anyway, this starts to get off topic for this PR and dropping CMAKE_CXX_STANDARD is not really urgent, so feel free to merge and postpone the discussion.

Sure, I will merge this pr as soon as CI is ready, but the conversation will die here most likely unless someone pushes on it.

I can see that we might want to switch to target_compile_features() + cxx_std_17, but dropping any mention of C++ standards in our cmake files doesn't seem to be the right way to go, again unless I'm missing something still.

wjwwood · 2022-05-13T02:08:54Z

Thanks for fixing it up :)

We're a little closer to being ready for C++20 I guess.

jspricke · 2022-05-13T08:34:15Z

I made a little example too, which shows that despite that, if one of your dependencies exports something like `target_compile_features(${PROJECT_NAME} PUBLIC cxx_std_17)`, but you use `target_compile_features(${PROJECT_NAME} PUBLIC cxx_std_11)` on your target, it will upgrade to C++17 anyway (overriding what you used on your target), which might be a problem if you're using something that has been removed since.

From my understanding upgrading is the correct behaviour. How should the compiler compile something that needs a newer standard?

> Also I just went through a lot of ROS 1 code to get the C++11 enforcement dropped ;). Why did you have to do that? Was there a build failure or did you just assume that it was out of date and therefore should be removed? (not an unreasonable assumption)

Because log4cxx in Debian testing and Ubuntu jammy needs C++17 and the way it is used in rosconsole results in all downstream projects to also need C++17.

>> Do you have a situation in mind? > This is open source so you can always work around it, sure :). So, no? I'm just trying to understand why it's a problem, because if it is, I'd like to pursue fixing it. It's written into the CMake in such a way as to encourage overriding it, so I was asking for a situation where you'd need to edit the source before building it.

CMAKE_CXX_STANDARD does different things depending on the compiler version and other components in the project. - for compilers that don't support the standard set by CMAKE_CXX_STANDARD it depends on CXX_STANDARD_REQUIRED. - With CXX_STANDARD_REQUIRED set to on it will fail. - With CXX_STANDARD_REQUIRED set to off if depends on if the compiler can compile the given project. There are cases where the compiler is fine with the code either because non of the newer language features are actually used or because the compiler happen to implement them or optimize them away. So not setting CXX_STANDARD_REQUIRED could result in success whereas setting it will always fail. - for compilers that support the new language standard but don't use it by default it is changed to compile with the new version. This is probably the reason it is used here in the first place but note that compilers have reasons not to enable it by default, for example because the implementation is not complete or well tested. This is the use case for target_compile_features. - for compilers defaulting to the requested standard it should not matter. - for compilers defaulting to newer standards it restricts the compilation to the older standard. If the project compiles with the new standard, CMAKE_CXX_STANDARD restricts the compiler without need. If the project is not valid with the new standard and can't be made to comply with it, setting CMAKE_CXX_STANDARD is maybe a valid option. From my experience this is not needed in open source projects. For most cases where I had to adopt code to a new standard the changes where tiny (except the ABI change in C++11, maybe). Also note that restricting CMAKE_CXX_STANDARD will exclude you from tests with newer compilers. For example in Debian we regularly integrate new compilers, test all packages against them and fix bugs (in the compiler and in the packages).

wjwwood · 2022-05-13T22:13:52Z

Because log4cxx in Debian testing and Ubuntu jammy needs C++17 and the
way it is used in rosconsole results in all downstream projects to also
need C++17.

From what I understood, if you used target_compile_features(${PROJECT_NAME} PUBLIC cxx_std_17) in rosconsole, then you wouldn't need to change anything else downstream (or upstream) of rosconsole.

wjwwood · 2022-05-13T22:14:43Z

I guess that assumes the use of exported targets... But you could also have used an extra file to ensure the CMAKE_CXX_STANDARD was set to at least 17.

jspricke · 2022-05-13T22:22:28Z

From what I understood, if you used `target_compile_features(${PROJECT_NAME} PUBLIC cxx_std_17)` in `rosconsole`, then you wouldn't need to change anything else downstream (or upstream) of `rosconsole`.

Not so easy. Some projects set CMAKE_CXX_STANDARD without testing if it was set before, others directly added -std=c++11 to the compiler flags. Anyway, I think the right way here was/is to just drop it and a lot of projects accepted it already.

wjwwood · 2022-05-13T22:31:31Z

This is
probably the reason it is used here in the first place but note that
compilers have reasons not to enable it by default, for example
because the implementation is not complete or well tested. This is the
use case for target_compile_features.

I mean that is why we're using it, and as I said, due to so many different people building it in so many different scenarios, it behooves us to set that as a low water mark. There's likely always a compiler in this situation. You can't just think about the latest Ubuntu or Debian version.

From my limited testing and reading the docs, target_compile_features and CMAKE_CXX_STANDARD work exactly the same, the only difference is that target_compile_features also supports PUBLIC/PRIVATE/INTERFACE style representation in exported targets.

I suppose we could check what version of C++ the compiler being used defaults to and only set target_compile_features if it is older than what we need, but that's a lot more complexity that honestly should be done by CMake...

for compilers defaulting to newer standards it restricts the
compilation to the older standard. If the project compiles with the
new standard, CMAKE_CXX_STANDARD restricts the compiler without need.
If the project is not valid with the new standard and can't be made to
comply with it, setting CMAKE_CXX_STANDARD is maybe a valid option.
From my experience this is not needed in open source projects. For
most cases where I had to adopt code to a new standard the changes
where tiny (except the ABI change in C++11, maybe).

I don't really see that as a downside. If your software is not using anything from the newer standards then there's no need to use the newer standard. It might be nice to notice what has changed, but until you start using it, you don't need it.

However, it would be great if CMake had a "CMAKE_CXX_MINIMUM_STANDARD" or something like that (equivalent for use in target_compile_features).

Also note that restricting CMAKE_CXX_STANDARD will exclude you from
tests with newer compilers. For example in Debian we regularly
integrate new compilers, test all packages against them and fix bugs
(in the compiler and in the packages).

You could (and probably should) set the CMAKE_CXX_STANDARD in that case... In that situation you cannot depend on every piece of software doing exactly as you would like, nor are they all using CMake.

Plus you are being tested with new compilers, what you mean to say is the implementation of new C++ standards with the new compiler. If the new compiler stops supporting -std=gnu++14 (or whatever), then I believe CMake will upgrade you to the next lowest one. Or maybe it refuses to build and you need to update the software. But I think that's a perfectly reasonable evolution of software.

Anyway, I think the right way here was/is to just drop it and a lot of
projects accepted it already.

We're going to have to just disagree about that. I think projects should declare what features of C++ they require, or at least which version of C++ they require. Furthermore, I think removing those declarations in the ROS 1 packages without replacing them with similar (but maybe more flexible or better) logic was a step backwards.

I could be convinced that there is a better way to do this, or that doing it based on features rather than standards is better (even if it's a lot more work), but I do not agree that hoping the underlying compiler is configured correctly to build your software is preferable. 🤷‍♂️

jspricke · 2022-05-13T22:45:45Z

I mean that is why we're using it, and as I said, due to so many different people building it in so many different scenarios, it behooves us to set that as a low water mark. There's likely always a compiler in this situation. You can't just think about the latest Ubuntu or Debian version.

Sure, and those with an old compiler probably have old versions of other libraries as well so they should probably use an old version of ROS. I don't think they should block ROS from moving forward. Software changes and the only way to cope with it is to adopt and move forward.

I suppose we could check what version of C++ the compiler being used defaults to and only set `target_compile_features` if it is older than what we need, but that's a lot more complexity that honestly should be done by CMake... However, it would be great if CMake had a "`CMAKE_CXX_MINIMUM_STANDARD`" or something like that (equivalent for use in `target_compile_features`).

This is what target_compile_features does.

From my limited testing and reading the docs, `target_compile_features` and `CMAKE_CXX_STANDARD` work exactly the same, the only difference is that `target_compile_features` also supports `PUBLIC`/`PRIVATE`/`INTERFACE` style representation in exported targets.

How did you test it that you think it does not?

Plus you are being tested with new compilers, what you mean to say is the implementation of new C++ standards with the new compiler. If the new compiler stops supporting `-std=gnu++14` (or whatever), then I believe CMake will upgrade you to the next lowest one. Or maybe it refuses to build and you need to update the software. But I think that's a perfectly reasonable evolution of software.

I guess you would need to update the software and that's what I did :).

We're going to have to just disagree about that. I think projects should declare what features of C++ they require, or at least which version of C++ they require. Furthermore, I think removing those declarations in the ROS 1 packages without replacing them with similar (but maybe more flexible or better) logic was a step backwards. I could be convinced that there is a better way to do this, or that doing it based on features rather than standards is better (even if it's a lot more work), but I do not agree that hoping the underlying compiler is configured correctly to build your software is preferable. 🤷‍♂️

Yes, let's agree to disagree.

clalancette · 2022-05-20T11:05:42Z

@Mergifyio backport humble

mergify · 2022-05-20T11:06:21Z

backport humble

✅ Backports have been created

#1937 Drop wrong template specialization (backport #1926) has been created for branch humble

This fails with g++ -std=gnu++20. Signed-off-by: Jochen Sprickerhof <git@jochen.sprickerhof.de> (cherry picked from commit 02802bc)

This fails with g++ -std=gnu++20. Signed-off-by: Jochen Sprickerhof <git@jochen.sprickerhof.de> (cherry picked from commit 02802bc) Co-authored-by: Jochen Sprickerhof <github@jochen.sprickerhof.de>

jspricke force-pushed the drop_wrong_template branch from bde63f5 to ca45f6e Compare May 10, 2022 21:22

griswaldbrooks reviewed May 10, 2022

View reviewed changes

fujitatomoya approved these changes May 11, 2022

View reviewed changes

wjwwood approved these changes May 11, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

Drop wrong template specialization

9e73982

This fails with g++ -std=gnu++20. Signed-off-by: Jochen Sprickerhof <git@jochen.sprickerhof.de>

jspricke force-pushed the drop_wrong_template branch from ca45f6e to 9e73982 Compare May 12, 2022 18:24

wjwwood approved these changes May 12, 2022

View reviewed changes

wjwwood merged commit 02802bc into ros2:master May 13, 2022

jspricke deleted the drop_wrong_template branch May 13, 2022 12:47

mergify bot mentioned this pull request May 20, 2022

Drop wrong template specialization (backport #1926) #1937

Merged

mergify bot pushed a commit that referenced this pull request May 20, 2022

Drop wrong template specialization (#1926)

123b5e3

This fails with g++ -std=gnu++20. Signed-off-by: Jochen Sprickerhof <git@jochen.sprickerhof.de> (cherry picked from commit 02802bc)

clalancette mentioned this pull request May 20, 2022

Backport "Drop wrong template specialization (#1926)" to Humble? #1935

Closed

fujitatomoya mentioned this pull request May 20, 2022

Unable to compile code that uses rclcpp::SubsciptionOptions with ROS Humble #1936

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop wrong template specialization #1926

Drop wrong template specialization #1926

jspricke commented May 10, 2022

jspricke commented May 10, 2022

griswaldbrooks left a comment

jspricke commented May 11, 2022

fujitatomoya commented May 11, 2022

fujitatomoya commented May 11, 2022

wjwwood left a comment

wjwwood commented May 11, 2022

griswaldbrooks commented May 12, 2022

griswaldbrooks commented May 12, 2022

wjwwood commented May 12, 2022

griswaldbrooks commented May 12, 2022

wjwwood commented May 12, 2022

This comment was marked as resolved.

griswaldbrooks commented May 12, 2022

wjwwood commented May 12, 2022

jspricke commented May 12, 2022

wjwwood commented May 12, 2022 •

edited

Loading

wjwwood commented May 12, 2022

jspricke commented May 12, 2022

wjwwood commented May 12, 2022

jspricke commented May 12, 2022 via email

wjwwood commented May 13, 2022

wjwwood commented May 13, 2022

jspricke commented May 13, 2022 via email

wjwwood commented May 13, 2022

wjwwood commented May 13, 2022

jspricke commented May 13, 2022 via email

wjwwood commented May 13, 2022

jspricke commented May 13, 2022 via email

clalancette commented May 20, 2022

mergify bot commented May 20, 2022

Drop wrong template specialization #1926

Drop wrong template specialization #1926

Conversation

jspricke commented May 10, 2022

jspricke commented May 10, 2022

griswaldbrooks left a comment

Choose a reason for hiding this comment

jspricke commented May 11, 2022

fujitatomoya commented May 11, 2022

fujitatomoya commented May 11, 2022

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood commented May 11, 2022

griswaldbrooks commented May 12, 2022

griswaldbrooks commented May 12, 2022

wjwwood commented May 12, 2022

griswaldbrooks commented May 12, 2022

wjwwood commented May 12, 2022

This comment was marked as resolved.

griswaldbrooks commented May 12, 2022

wjwwood commented May 12, 2022

jspricke commented May 12, 2022

wjwwood commented May 12, 2022 • edited Loading

wjwwood commented May 12, 2022

jspricke commented May 12, 2022

wjwwood commented May 12, 2022

jspricke commented May 12, 2022 via email

wjwwood commented May 13, 2022

wjwwood commented May 13, 2022

jspricke commented May 13, 2022 via email

wjwwood commented May 13, 2022

wjwwood commented May 13, 2022

jspricke commented May 13, 2022 via email

wjwwood commented May 13, 2022

jspricke commented May 13, 2022 via email

clalancette commented May 20, 2022

mergify bot commented May 20, 2022

✅ Backports have been created

wjwwood commented May 12, 2022 •

edited

Loading