Opened on 07/11/2014 at 12:34:03 PM

Closed on 08/25/2015 at 02:34:31 PM

#768 closed change (fixed)

Switch from TR1 to C++11

Reported by: trev Assignee:
Priority: P4 Milestone:
Module: Libadblockplus Keywords:
Cc: fhd, rjeschke, serggzz@gmail.com 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.

Attachments (0)

Change History (13)

comment:1 Changed on 07/11/2014 at 12:36:51 PM by trev

  • Platform changed from Unknown to Android

comment:2 Changed on 07/11/2014 at 12:38:40 PM 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 on 07/11/2014 at 12:42:46 PM 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 on 07/11/2014 at 02:29:27 PM by trev

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

comment:5 Changed on 07/11/2014 at 02:32:42 PM by trev

  • Blocking 769 added

comment:6 Changed on 08/28/2014 at 07:29:24 AM 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 on 09/04/2014 at 10:57:35 PM 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 on 09/04/2014 at 11:00:02 PM by fhd

comment:8 Changed on 09/05/2014 at 02:16:15 PM 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 on 09/05/2014 at 02:16:43 PM by sergz

  • Cc serggzz@gmail.com added

comment:10 Changed on 09/05/2014 at 05:53:24 PM 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 on 09/05/2014 at 06:15:07 PM by sergz

We could easily do this with a define.

The example of the trick is here

comment:12 Changed on 06/23/2015 at 11:10:14 AM 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 on 08/25/2015 at 02:34:31 PM by sergz

  • Resolution set to fixed
  • Status changed from reviewing to closed
  • Tester set to Unknown

Add Comment

Modify Ticket

Change Properties
Action
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from (none).
 
Note: See TracTickets for help on using tickets.