#6063 closed change (fixed)

Start to use C++14

Reported by: sergz Assignee: sergz
Priority: P2 Milestone:
Module: Libadblockplus Keywords:
Cc: asmirnov, hfiguiere Blocked By:
Blocking: #6588 Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29748587/

Description

Background

We should at least ensure that our headers are compatible with C++14 in order to give a possibility to use libadblockplus in modern applications. Additionally it would be good have an easy way of compiling the library with different C++ standards.

Change History (20)

comment:1 Changed 22 months ago by asmirnov

For using libadblockplus-android in Chromium we should compile libadblockplus with exactly the same version of v8 that chromium app uses. At the moment we compile for c++11. Starting recent times (actually starting that change https://chromium-review.googlesource.com/c/v8/v8/+/728026) c++14 is required.

Note that v8 inherits the style guide from chromium, and chromium allows to use C++14 feature: https://chromium-cpp.appspot.com/.

Probably we could revert some files to still use c++11 but i believe that it would be better to support C++14. BTW I was able to compile chromium with v8 revision e57e7b81b8584ddbd5268b8e7af2988e4f87fc84 that uses C++14.

I've tried to edit common.gypi and replace -std=c++11 with -std=c++14 and 'CLANG_CXX_LANGUAGE_STANDARD': 'c++11' with 'CLANG_CXX_LANGUAGE_STANDARD': 'c++14' but it introduced new build errors:

     /Users/asmirnov/Library/Android/ndk/android-ndk-r12b/sources/cxx-stl/llvm-libc++/libcxx/include/tuple:356:72: error: with 'constexpr _Hp& std::__ndk1::__tuple_leaf<_Ip, _Hp, true>::get() const'
     _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX11       _Hp& get()       _NOEXCEPT {return static_cast<_Hp&>(*this);}

similar issue https://github.com/android-ndk/ndk/issues/174

comment:2 follow-up: Changed 22 months ago by asmirnov

BTW it would be great to check compilation with the newest android NDK, which is 16. At the moment we still use 12b.

comment:3 in reply to: ↑ 2 ; follow-up: Changed 22 months ago by sergz

Replying to asmirnov:

BTW it would be great to check compilation with the newest android NDK, which is 16. At the moment we still use 12b.

This dependency comes from V8.

comment:4 in reply to: ↑ 3 Changed 22 months ago by asmirnov

Replying to sergz:

Replying to asmirnov:

BTW it would be great to check compilation with the newest android NDK, which is 16. At the moment we still use 12b.

This dependency comes from V8.

Yeah, i can see android_tools in Chromium still having NDK 12b so probably that's the limit.

Last edited 22 months ago by asmirnov (previous) (diff)

comment:5 Changed 21 months ago by sergz

I think we should try to update V8 (according to https://omahaproxy.appspot.com/ we can do it again, the prev update was #5698) to a version containing required changes and either at the same time or right after the update switch to C++14 if it's possible (the latter means supported by the compiler from the ndk).

comment:6 Changed 20 months ago by asmirnov

64.0.3249.2 is the last version of Chromium where we can build libadblockplus while building of Chromium, it uses V8 of 6.4.102 .version.

In Chromium 64.0.3250.0 v8 of version 6.4.126 is used and it requires C++14 to be supported.
Actually C++ is required starting V8 of version 6.4.107: https://chromium.googlesource.com/v8/v8.git/+/refs/heads/6.4.107.

So if we we'd like to update to Chromium 64.0.3250.0 this ticket will be the blocker.

However we could probably solve it by avoid building of V8 for libadblockplus as we can just reuse Chromium V8 build files. For this we need #5503 to be solved.

comment:7 Changed 18 months ago by asmirnov

Now it's blocking some of our partners to use Chromium 65.

comment:8 Changed 18 months ago by sergz

  • Priority changed from P4 to P2

comment:9 Changed 18 months ago by sergz

The code of libadblockplus, abpshell and tests can be compiled with -std=c++14 (modified common.gypi) on linux.

I suppose that it also works with Visual C++ since we are using relatively recent platform toolsets, e.g. v141. If we decide to limit ourselves to, e.g. C++14, then please pay attention to the compiler flag /std:c++14 what can be specified in common.gypi as (not tested)

'msvs_settings': {
  'VCCLCompilerTool': { 'AdditionalOptions': ['/std:c++14'], },
},

The support of C++14 by V8 and by android ndk is not tested yet.

comment:10 Changed 18 months ago by sergz

  • Owner set to sergz

I'm observing the same issue as in comment:1 trying to compile by ndk, no matter only libadblockplus or v8.

So, one need to see whether it will be resolved by updating of V8.

comment:11 Changed 18 months ago by sergz

When one selects -std=c++14 then some features are not supported by the compiler from the ndk r12b. So it seems that the only option is to switch to a new ndk, presumable r16b.

Basically the everything can be compiled by r16b but there are some linking errors. I'm looking into it.

Additionally there is a non-standard linker chromium project, so one has to check that that linker is able to link the code compiled by r16b and that it is still properly working.

comment:12 Changed 18 months ago by sergz

  • Owner sergz deleted

Good news: it's possible to obtain a binary using NDK r16b, C++14, the version of V8 6.5.254.41 and gyp, though with several manipulations (The most tricky one was to drop the linker param -Wl,--fatal-warnings). And this binary is actually running on a device.

So, since we know that the code is compatible with the compiler, IMO now we should rather focus on the building system, namely prioritize looking into using GN to build V8 than to create hacks in the current building system, if there is time for that.

---
I'm unassigned myself so far, we will firstly discuss the priorities, because my involvement in this issue was rather an ad-hoc.

comment:13 Changed 18 months ago by asmirnov

so what should be changed in gyp files to compile it with make android_arm with NDK 16b (except passing correct path to NDK)?

comment:14 Changed 18 months ago by hfiguiere

the path to the stdlib have changed in the NDK 16. standalone.gypi need to be changed.

For example:

'android_libcpp_include': '<(android_stl)/llvm-libc++/libcxx/include',
'android_libcpp_abi_include': '<(android_stl)/llvm-libc++abi/libcxxabi/include',

needs to be changed to

'android_libcpp_include': '<(android_stl)/llvm-libc++/include',
'android_libcpp_abi_include': '<(android_stl)/llvm-libc++abi/include',

(probably not the only place)

comment:15 Changed 18 months ago by hfiguiere

  • Cc hfiguiere added

comment:16 Changed 18 months ago by sergz

  • Owner set to sergz

Yep, I will firstly send the required changes to some repository and then we will see.
BTW, the same changes should be done even in .gn files.

comment:17 Changed 17 months ago by sergz

  • Review URL(s) modified (diff)

comment:18 Changed 17 months ago by asmirnov

  • Blocking 6588 added

comment:19 Changed 17 months ago by abpbot

A commit referencing this issue has landed:
Issue 6063, 6531 - Update V8 to 6.5.254.41, use C++14 and NDK-r16b

comment:20 Changed 17 months ago by sergz

  • Resolution set to fixed
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.