Opened 5 years ago

Closed 4 years ago

#768 closed change (fixed)

Switch from TR1 to C++11

Reported by: trev Assignee:
Priority: P4 Milestone:
Module: Libadblockplus Keywords:
Cc: fhd, rjeschke, serggzz@… Blocked By:
Blocking: #769 Platform: Unknown
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

http://codereview.adblockplus.org/5163715573841920/

Description (last modified by trev)

Background

Currently we have to use newer features like shared_ptr only with tr1 prefix, features like unique_ptr cannot be used at all.

What to change

Enable C++11 for our code. To do this we need to replace libstdc++ by libc++ for Android - the former cannot be linked statically due to licensing issues, which means that we depend on whichever version is installed on the system - and that one can be very outdated for some Android devices. Notes:

  • libc++ was only recently added to the Android NDK, most likely with Android NDK r9d. We'll need to update requirements.
  • It seems that the GCC version in Android NDK is too old for libc++ - GCC 4.6 says "sorry, unimplemented: mangling dotstar_expr" (seems to have been fixed in GCC 4.7). clang works however.
  • We need to let googletest determine itself whether tuple header is available rather than overriding the decision with macros.
  • In shell/src/Main.cpp the result of std::getline() cannot be converted to bool - we were probably exploiting an implementation detail of libstdc++ there.

Change History (13)

comment:1 Changed 5 years ago by trev

  • Platform changed from Unknown to Android

comment:2 Changed 5 years ago by fhd

Note that we also need to replace everything from TR1 we use with the C++11 equivalents in the same step. libc++ doesn't have TR1 support, it's a C++11+ library. (We can keep auto_ptr for now though, even though we should soon replace it with unique_ptr once we moved to C++11.)

comment:3 Changed 5 years ago by trev

  • Description modified (diff)
  • Platform changed from Android to Unknown
  • Summary changed from Use libc++ instead of libstdc++ to Switch from TR1 to C++11

comment:4 Changed 5 years ago by trev

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:5 Changed 5 years ago by trev

  • Blocking 769 added

comment:6 Changed 5 years ago by fhd

  • Cc rjeschke added

One thing we didn't consider enough here I think: What about client applications written in C++98/03? As it stands, they won't be able to use libadblockplus once we merge this.

Note that TR1 isn't particularly portable either - has never been as widely supported as C++03, and compilers are starting to drop support for it. Boost can serve as a drop-in replacement for TR1 (which largely came from Boost). Adding a huge dependency isn't nice, but this would be the easiest way to arrive at something highly portable.

Adding René - what do you think?

comment:7 Changed 5 years ago by fhd

I've just confirmed that all the major libadblockplus clients should be able to use C++11, so I suppose we could merge this.

However, I'd like to consider the alternatives that would keep us C++98 compatible. Here's the stuff we use in headers that client applications will include:

  1. std::shared_ptr
  2. std::enable_shared_from_this
  3. std::function

The only way to stay C++98 compatible will be to eliminate this stuff from the public API. (We can still go nuts with C++11 features as we see fit in our implementation as long as the standard library is linked statically).

I think we have two reasonable options for this:

  1. Use that stuff from boost - it's zero effort since it'll actually be a drop-in replacement. The major downside is that it'll add megabytes worth of headers to the project even when using bcp, which is supposed to only copy individual boost libraries. However, I'm pretty sure we can get away with a hand full of headers if we copy what we need manually (I think bcp will just copy all of Boost.Core).
  1. Write our own replacements. std::shared_ptr is a textbook excercise. std::enable_shared_from_this sounds doable, too. Replacing std::function could be as simple as using a plain old functor in the public API.

Neither option sounds really brilliant. But frankly, it sounds better to me than to make it impossible for most C++ projects out there to use libadblockplus overnight.

Last edited 5 years ago by fhd (previous) (diff)

comment:8 Changed 5 years ago by sergz

I'm also not aware about the clients which don't support used features of C++11. If there are some concrete troubles, could somebody please post them here?
As I understand there is no troubles at the linking stage now and the client code is compiled by the same compiler as the library code. So I would suggest to keep the code clean without compatibility "polyfills" so far.

I think we have two reasonable options for this:

  1. Use that stuff from boost...
  2. Write our own replacements...

I would add another option, we can access them from our own namespace. For example with the shared_ptr like abp::shared_ptr and in our own namespace we can choose between std::shared_ptr, std::tr1::shared_ptr and boost::shared_ptr, we can leave it up to the client. As well for the enums as far as I know recent compilers allow to use enum type name as prefix like in C++11, thus MyEnumType::valueX without emulating of scoped enums.

But so far there is no concrete troubles I would consider all this options like overengineering.

comment:9 Changed 5 years ago by sergz

  • Cc serggzz@… added

comment:10 Changed 5 years ago by fhd

That's a great solution Sergei! All we'd have to do is import whatever the user wants (C++11, TR1 or Boost) into our own namespace and that's it. We could easily do this with a define.

But I suppose I'm the only one thinking we should stay backwards compatible just-in-case, so I guess we should do it the way you suggested: Require C++11 and see if it's a problem for anyone. I'm happy that we have an easy workaround in case this happens.

We should definitely point this out in the README though. Will say that in the review.

comment:11 Changed 5 years ago by sergz

We could easily do this with a define.

The example of the trick is here

comment:12 Changed 4 years ago by trev

  • Owner trev deleted

Unassigning, feel free to take it as I won't finish this any more. I can add you as a "contributor" to the existing review.

comment:13 Changed 4 years ago by sergz

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