Switch to C++20

[kkarbowiak]

Issue #851

Switch to C++20.

Tasks

• Update CHANGELOG.md (remove if irrelevant)
• review

[kkarbowiak]

This change produces quite a lot of warnings even on GCC (which usually seems less picky than Clang), so I'm leaving this PR as a draft to have a closer look at those.

[jcoupey]

Thanks for giving this a go. Maybe we could enable -Werror for this PR in order to have the build fail until we have all warnings sorted out?

[kkarbowiak]

Maybe we could enable -Werror for this PR in order to have the build fail until we have all warnings sorted out?

Agreed. Also, once the build is free of warnings we could leave the -Werror flag enabled.

Regarding the warnings themselves, all of them originate in external libraries: rapidjson and polylineencoder. Perhaps just updating to newer versions would fix those.

[jcoupey]

Thanks for enabling the flag.

Also, once the build is free of warnings we could leave the -Werror flag enabled

That was also my first idea but I have to admit that this article makes a quite convincing point.

all of them originate in external libraries: rapidjson and polylineencoder

Then that's a different problem. I'd expect suppressing warnings in polylineencoder should be pretty straightforward, and the changes would probably be gladly accepted upstream.

On the other hand I don't think we could have much control on rapidjson itself. The version we ship is old: basically v1.1.0 from 2016 (!!) with a couples patches. The amount of work to silence warnings with a recent compiler on C+++20 is probably quite significant.

[jcoupey]

I'd expect suppressing warnings in polylineencoder should be pretty straightforward, and the changes would probably be gladly accepted upstream.

Wow, that was fast! vahancho/polylineencoder#14 + https://github.com/vahancho/polylineencoder/releases/tag/v2.0.1

[kkarbowiak]

Wow, that was fast! vahancho/polylineencoder#14 + https://github.com/vahancho/polylineencoder/releases/tag/v2.0.1

I'm myself surprised :-)

On the other hand I don't think we could have much control on rapidjson itself. The version we ship is old: basically v1.1.0 from 2016 (!!) with a couples patches. The amount of work to silence warnings with a recent compiler on C+++20 is probably quite significant.

What is the upstream repo? Is it https://github.com/Tencent/rapidjson? If so, the latest release is still 1.1.0, but there have been some development since.

[jcoupey]

What is the upstream repo? Is it https://github.com/Tencent/rapidjson? If so, the latest release is still 1.1.0

Yes, for the record there is an epic thread over there at issue 1006 dating back from 2017 (!) basically asking for a new release that never happened so far.

AFAICT by using my memory and diffing the upstream repo and our include/rapidjson folder we're basically on v1.1.0, updated from a previous version in c21c290. Only document.h has been modified after that by be7f44b, cherry-picking an upstream change in order to... silence some warnings! So we're back at the same place a few years later.

There have been 725 (!!) commits to the master branch there since the last release so maybe we should take advantage of new fixes and changes, but then it would feel weird and random to pin to any (non-release) commit over there...

I guess one of the ways to decide if it's worth doing a wild upgrade would be to check if it would fix the C++20 warnings in the first place?

[kkarbowiak]

Yes, for the record there is an epic thread over there at issue 1006 dating back from 2017 (!) basically asking for a new release that never happened so far.

I see.

AFAICT by using my memory and diffing the upstream repo and our include/rapidjson folder we're basically on v1.1.0, updated from a previous version in c21c290. Only document.h has been modified after that by be7f44b, cherry-picking an upstream change in order to... silence some warnings! So we're back at the same place a few years later.

There have been 725 (!!) commits to the master branch there since the last release so maybe we should take advantage of new fixes and changes, but then it would feel weird and random to pin to any (non-release) commit over there...

I agree that taking something that even the authors do not think warrants a tag is risky and uncomfortable at best.

I guess one of the ways to decide if it's worth doing a wild upgrade would be to check if it would fix the C++20 warnings in the first place?

This is a good idea. I will give it a go.

[jcoupey]

I've forced a re-run on the job that previously failed due to install errors. Now we get a proper failure due to a warning while building vroom, but it's still a problem with polylineencoder since the version from the git submodule is still the same.

[kkarbowiak]

After updating polylineencoder to v2.0.1 both g++ and clang++ builds are clean.

[jcoupey]

Thanks for testing this, now we know that we can easily get away with the warnings if we update. I guess it now comes down to 2 options:

1. We take the opportunity of the switch to C++20 to also update rapidjson, in which case we may benefit from upstream fixes and improvements. The drawback is that we have to pin a random upstream version from master. Apart from feeling very weird we have no confidence that it won't break anything while after all the current version works very well for us.
2. We decide that the C++20 switch has more to do with our own codebase and de-corellate the rapidjson update altogether, after all we have no real need to upgrade feature-wise. This means we either have to live with a lot of extra warnings coming for third-party code (not realistic), or find a way to disable warnings for that portion of code (not sure this is even possible).

[jcoupey]

@kkarbowiak sorry for the delay here. I finally went with option 1. and updated Rapidjson (now as a submodule). Most of the changes in this PR have been necessary for experimentation but are now somehow void. Shall we close and open another PR?

I also ticketed the necessary polylineencoder update in #931.

[kkarbowiak]

@kkarbowiak sorry for the delay here.

No worries. I was also focusing on some other stuff.

I finally went with option 1. and updated Rapidjson (now as a submodule). Most of the changes in this PR have been necessary for experimentation but are now somehow void. Shall we close and open another PR?

Alright. This PR is still a draft and I'm ok with closing it.

I also ticketed the necessary polylineencoder update in #931.

Cool.