Update to C++17 #560

kheaactua · 2023-11-03T16:25:38Z

Modernizing to C++17

Address compiler warnings:

Fixing narrowing issues
Removing useless copies
Removing unused lines
unused-lambda-capture
Removes unused variables
Fixes some casts
Explicitly deleting unused endpoint_impl copy and move constructors
Removing redundant std::bind
Improving const correctness
Moving thread init to constructor body
Moved check_routing_credentials_ inside vsomeip security section where
it's used
Using =default destructor instead of empty destructor

Thread init:

Moving the initialization of these threads into the constructor body to ensure that they do not start with an incomplete "this". As they capture this, it is possible that if the new thread begins before the object is fully constructed, the new thread might operate on uninitialized members of "this".

kheaactua · 2023-11-03T16:26:04Z

This PR does not address all the warnings, but it does address a good number of them

fcmonteiro · 2023-11-06T13:26:28Z

HI @kheaactua ,
Any specific reason you need to use C++17?
We will analyse this internally, but we have been trying to avoid making the move to C++17.
Thanks

kheaactua · 2023-11-06T13:46:09Z

There's no strictly compelling reason. However it does provide advantages such as constexpr.

We use AOSP (C++20) and QNX 7.1 (C++17), this commit was first created for 3.1.20 which actually had C++17 incompatible code (configuration_impl::trim)

I can re-submit this PR without the C++17 specific changes if that's preferred. I left them in this submission just in case Covesa was interesting in the language standard change. (I'm not sure when dependencies such as boost or asio will start requiring more than C++11)

I can also switch the code to west-side const (I see the preference for that in 9952fea)

goncaloalmeida · 2023-11-20T15:40:06Z

we will check this internally @kheaactua

CMakeLists.txt

- Fixing narrowing issues - Removing useless copies - Removing unused lines - unused-lambda-capture - Removes unused variables - Fix some casts (modernize c-style, or simply remove useless casts) - Explicitly deleting unused endpoint_impl copy and move constructors - Removing redundant std::bind - Improving const correctness - Moving thread init to constructor body - Moved check_routing_credentials_ inside vsomeip security section where it's used - Using =default destructor instead of empty destructor Thread init: Moving the initialization of these threads into the constructor body to ensure that they do not start with an incomplete "this". As they capture this, it is possible that if the new thread begins before the object is fully constructed, the new thread might operate on uninitialized members of "this".

goncaloalmeida · 2023-11-29T13:10:06Z

@kheaactua with force push it is not easy for the reviews and this will take more time.

kheaactua · 2023-11-29T13:24:16Z

@kheaactua with force push it is not easy for the reviews and this will take more time.

Oh, sorry about that. It's SOP at work so I suppose just a habit. Plus I noticed some work jira IDs in the commit messages that I wanted to remove.

Now that I know I won't force push in the future.

fcmonteiro · 2024-01-05T11:25:45Z

Hi @kheaactua, can you have a look at the failing windows build?
We are interested in merging this PR.
Thanks

kheaactua · 2024-01-05T15:08:14Z

Hi @kheaactua, can you have a look at the failing windows build? We are interested in merging this PR. Thanks

Will do! I thought I had but I suppose that was a different PR.

kheaactua · 2024-01-09T18:04:39Z

Hi @kheaactua, can you have a look at the failing windows build? We are interested in merging this PR. Thanks

@fcmonteiro All tests pass!

cbangalore · 2024-01-11T10:51:04Z

@kheaactua I tried to create a draft PR with all the changes as per this PR and looks like there is compatibility issue with boost-1.65.0 and the build fails for boost 1.65 whereas there is no issue with the boost-1.83.0 and it builds with success

fcmonteiro · 2024-01-11T11:21:58Z

@kheaactua I tried to create a draft PR with all the changes as per this PR and looks like there is compatibility issue with boost-1.65.0 and the build fails for boost 1.65 whereas there is no issue with the boost-1.83.0 and it builds with success

@kheaactua , part of the failing log,

[  2%] Building CXX object CMakeFiles/vsomeip3.dir/implementation/endpoints/src/endpoint_definition.cpp.o
In file included from /home/zuul/boost/include/boost/icl/type_traits/identity_element.hpp:11,
                 from /home/zuul/boost/include/boost/icl/type_traits/unit_element.hpp:12,
                 from /home/zuul/boost/include/boost/icl/concept/interval.hpp:17,
                 from /home/zuul/boost/include/boost/icl/concept/joinable.hpp:12,
                 from /home/zuul/boost/include/boost/icl/associative_interval_container.hpp:13,
                 from /home/zuul/boost/include/boost/icl/interval_base_set.hpp:24,
                 from /home/zuul/boost/include/boost/icl/interval_set.hpp:14,
                 from /home/zuul/src/internal-github.net/networking/vsomeip-lib/implementation/endpoints/src/../include/../../configuration/include/configuration.hpp:16,
                 from /home/zuul/src/internal-github.net/networking/vsomeip-lib/implementation/endpoints/src/../include/endpoint_impl.hpp:18,
                 from /home/zuul/src/internal-github.net/networking/vsomeip-lib/implementation/endpoints/src/../include/client_endpoint_impl.hpp:24,
                 from /home/zuul/src/internal-github.net/networking/vsomeip-lib/implementation/endpoints/src/client_endpoint_impl.cpp:20:
/home/zuul/boost/include/boost/icl/type_traits/type_to_string.hpp:56:12: error: partial specialization of ‘struct boost::icl::type_to_string<Unary<Type> >’ after instantiation of ‘struct boost::icl::type_to_string<std::__cxx11::basic_string<char> >’ [-fpermissive]
   56 |     struct type_to_string<Unary<Type> >
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/zuul/boost/include/boost/icl/type_traits/type_to_string.hpp:72:12: error: partial specialization of ‘struct boost::icl::type_to_string<Binary<Type1, Type2> >’ after instantiation of ‘struct boost::icl::type_to_string<std::__cxx11::basic_string<char> >’ [-fpermissive]
   72 |     struct type_to_string<Binary<Type1, Type2> >
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[  3%] Building CXX object CMakeFiles/vsomeip3.dir/implementation/endpoints/src/endpoint_impl.cpp.o
gmake[2]: *** [CMakeFiles/vsomeip3.dir/build.make:76: CMakeFiles/vsomeip3.dir/implementation/endpoints/src/client_endpoint_impl.cpp.o] Error 1
gmake[2]: *** Waiting for unfinished jobs....
In file included from /home/zuul/boost/include/boost/icl/type_traits/identity_element.hpp:11,
                 from /home/zuul/boost/include/boost/icl/type_traits/unit_element.hpp:12,
                 from /home/zuul/boost/include/boost/icl/concept/interval.hpp:17,
                 from /home/zuul/boost/include/boost/icl/concept/joinable.hpp:12,
                 from /home/zuul/boost/include/boost/icl/associative_interval_container.hpp:13,
                 from /home/zuul/boost/include/boost/icl/interval_base_set.hpp:24,
                 from /home/zuul/boost/include/boost/icl/interval_set.hpp:14,
                 from /home/zuul/src/internal-github.net/networking/vsomeip-lib/implementation/endpoints/src/../include/../../configuration/include/configuration.hpp:16,
                 from /home/zuul/src/internal-github.net/networking/vsomeip-lib/implementation/endpoints/src/../include/endpoint_impl.hpp:18,
                 from /home/zuul/src/internal-github.net/networking/vsomeip-lib/implementation/endpoints/src/endpoint_impl.cpp:20:
/home/zuul/boost/include/boost/icl/type_traits/type_to_string.hpp:56:12: error: partial specialization of ‘struct boost::icl::type_to_string<Unary<Type> >’ after instantiation of ‘struct boost::icl::type_to_string<std::__cxx11::basic_string<char> >’ [-fpermissive]
   56 |     struct type_to_string<Unary<Type> >
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/zuul/boost/include/boost/icl/type_traits/type_to_string.hpp:72:12: error: partial specialization of ‘struct boost::icl::type_to_string<Binary<Type1, Type2> >’ after instantiation of ‘struct boost::icl::type_to_string<std::__cxx11::basic_string<char> >’ [-fpermissive]
   72 |     struct type_to_string<Binary<Type1, Type2> >
      |            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
gmake[2]: *** [CMakeFiles/vsomeip3.dir/build.make:118: CMakeFiles/vsomeip3.dir/implementation/endpoints/src/endpoint_impl.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:191: CMakeFiles/vsomeip3.dir/all] Error 2
gmake: *** [Makefile:146: all] Error 2

kheaactua · 2024-01-11T14:52:58Z

@kheaactua I tried to create a draft PR with all the changes as per this PR and looks like there is compatibility issue with boost-1.65.0 and the build fails for boost 1.65 whereas there is no issue with the boost-1.83.0 and it builds with success

@kheaactua , part of the failing log,

Fun... Okay, I'll take a look. Thanks for the log!

kheaactua · 2024-01-11T16:49:01Z

@kheaactua I tried to create a draft PR with all the changes as per this PR and looks like there is compatibility issue with boost-1.65.0 and the build fails for boost 1.65 whereas there is no issue with the boost-1.83.0 and it builds with success

What compiler was used? I'm able to build this branch with boost 1.65 using Clang 17.0.6 on Linux.

cbangalore · 2024-01-16T10:33:25Z

Clang 17.0.6 on Linux

CXX compiler identification is GNU 11.4.0

kheaactua · 2024-01-16T14:05:06Z

Clang 17.0.6 on Linux

CXX compiler identification is GNU 11.4.0

Great, I'll spin up a container and try the build again

kheaactua · 2024-01-22T16:48:36Z

(still haven't had a chance to test with gcc 11 / boost 1.55, will do though)

fcmonteiro · 2024-02-02T17:37:22Z

We have found the issue to be Boost (1.65) Interval Container Library (ICL) does not compile with C++17, because in C++17 there was a change regarding templates (Please refer to this page, "New template template-parameter matching").
With GCC we can use the flag -fno-new-ttp-matching to revert the change, however there is no alternative for clang.

We are still analyzing this and making some cleanups internally, but we will probably bump the minimum Boost support to 1.66 which already fix this problem.

BrunoLSilvaCTW

Hi @kheaactua,

Changes look good. Just need to fix some style issues.

implementation/configuration/src/configuration_impl.cpp

implementation/endpoints/src/endpoint_manager_base.cpp

implementation/endpoints/src/local_uds_client_endpoint_impl.cpp

implementation/endpoints/src/local_uds_server_endpoint_impl.cpp

implementation/service_discovery/src/service_discovery_impl.cpp

implementation/configuration/src/configuration_impl.cpp

kheaactua · 2024-04-04T14:55:48Z

I think I addressed all the comment. In the last push it looked like C/C++ CI / build_on_ubuntu_boost_183_gcc_x86 (pull_request) failed.

error: narrowing conversion of ‘its_device.std::__cxx11::basic_string<char>::size()’ from ‘std::__cxx11::basic_string<char>::size_type’ {aka ‘long unsigned int’} to ‘socklen_t’ {aka ‘unsigned int’} [-Werror=narrowing]
  181 |                     SOL_SOCKET, SO_BINDTODEVICE, its_device.c_str(), socklen_t{its_device.size()}) == -1) {
      |

Looks like it'll have to be a static_cast in order to force the narrowing.

BrunoLSilvaCTW

LGTM

kheaactua force-pushed the switch_cpp17 branch from 22fdbb1 to 4d9b862 Compare November 3, 2023 16:29

kheaactua marked this pull request as draft November 3, 2023 17:11

kheaactua marked this pull request as ready for review November 4, 2023 14:26

goncaloalmeida requested changes Nov 21, 2023

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

kheaactua added 2 commits November 27, 2023 13:19

Upgrading to C++17

2960d2c

kheaactua force-pushed the switch_cpp17 branch from 4d9b862 to 9ac4191 Compare November 27, 2023 18:20

Merge branch 'master' into switch_cpp17

5324772

Attempting to fix syntax error on MSVC

8398044

kheaactua requested a review from goncaloalmeida January 9, 2024 18:04

kheaactua mentioned this pull request Mar 22, 2024

Initialize the routing running state #655

Open

BrunoLSilvaCTW requested changes Mar 26, 2024

View reviewed changes

Adjusting PR to conform to Covesa style

cced1f3

kheaactua added 3 commits April 4, 2024 10:57

Using curly brace initialization

ff8ec66

Using static_cast to narrow its_device.size() to a socklen_t

00398bc

Avoided double integer promotion

2c85a8a

kheaactua force-pushed the switch_cpp17 branch from 1822b45 to 2c85a8a Compare April 4, 2024 15:00

BrunoLSilvaCTW approved these changes Apr 5, 2024

View reviewed changes

goncaloalmeida approved these changes Apr 10, 2024

View reviewed changes

goncaloalmeida merged commit cf49723 into COVESA:master Apr 10, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to C++17 #560

Update to C++17 #560

kheaactua commented Nov 3, 2023 •

edited

kheaactua commented Nov 3, 2023

fcmonteiro commented Nov 6, 2023

kheaactua commented Nov 6, 2023 •

edited

goncaloalmeida commented Nov 20, 2023

goncaloalmeida commented Nov 29, 2023

kheaactua commented Nov 29, 2023

fcmonteiro commented Jan 5, 2024

kheaactua commented Jan 5, 2024

kheaactua commented Jan 9, 2024

cbangalore commented Jan 11, 2024

fcmonteiro commented Jan 11, 2024

kheaactua commented Jan 11, 2024

kheaactua commented Jan 11, 2024 •

edited

cbangalore commented Jan 16, 2024

kheaactua commented Jan 16, 2024

kheaactua commented Jan 22, 2024

fcmonteiro commented Feb 2, 2024

BrunoLSilvaCTW left a comment

kheaactua commented Apr 4, 2024 •

edited

BrunoLSilvaCTW left a comment

Update to C++17 #560

Update to C++17 #560

Conversation

kheaactua commented Nov 3, 2023 • edited

Address compiler warnings:

Thread init:

kheaactua commented Nov 3, 2023

fcmonteiro commented Nov 6, 2023

kheaactua commented Nov 6, 2023 • edited

goncaloalmeida commented Nov 20, 2023

goncaloalmeida commented Nov 29, 2023

kheaactua commented Nov 29, 2023

fcmonteiro commented Jan 5, 2024

kheaactua commented Jan 5, 2024

kheaactua commented Jan 9, 2024

cbangalore commented Jan 11, 2024

fcmonteiro commented Jan 11, 2024

kheaactua commented Jan 11, 2024

kheaactua commented Jan 11, 2024 • edited

cbangalore commented Jan 16, 2024

kheaactua commented Jan 16, 2024

kheaactua commented Jan 22, 2024

fcmonteiro commented Feb 2, 2024

BrunoLSilvaCTW left a comment

Choose a reason for hiding this comment

kheaactua commented Apr 4, 2024 • edited

BrunoLSilvaCTW left a comment

Choose a reason for hiding this comment

kheaactua commented Nov 3, 2023 •

edited

kheaactua commented Nov 6, 2023 •

edited

kheaactua commented Jan 11, 2024 •

edited

kheaactua commented Apr 4, 2024 •

edited