Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29419623: Issue 5165 - Remove SubscriptionPtr (Closed)

Created:
April 21, 2017, 2:08 p.m. by hub
Modified:
April 25, 2017, 12:02 p.m.
Reviewers:
sergei
Base URL:
https://hg.adblockplus.org/libadblockplus/
Visibility:
Public.

Description

Issue 5165 - Remove SubscriptionPtr

Patch Set 1 #

Patch Set 2 : Make Subscription(JsValue&&) protected #

Patch Set 3 : Added move constructor. #

Total comments: 5

Patch Set 4 : Added copy ctor. Don't use = default. #

Total comments: 2

Patch Set 5 : Added arg name in declaration #

Patch Set 6 : Added move assignment operator #

Patch Set 7 : Call the inherited operator #

Unified diffs Side-by-side diffs Delta from patch set Stats (+136 lines, -97 lines) Patch
M include/AdblockPlus/FilterEngine.h View 1 2 3 4 5 3 chunks +25 lines, -8 lines 0 comments Download
M shell/src/SubscriptionsCommand.cpp View 2 chunks +16 lines, -16 lines 0 comments Download
M src/FilterEngine.cpp View 1 2 3 4 5 6 2 chunks +30 lines, -8 lines 0 comments Download
M test/FilterEngine.cpp View 1 2 3 6 chunks +65 lines, -65 lines 0 comments Download

Messages

Total messages: 11
hub
April 21, 2017, 2:08 p.m. (2017-04-21 14:08:06 UTC) #1
sergei
Could you please make constructor accepting JsValue&& as protected, add FilterEngine as a friend class, ...
April 21, 2017, 3:19 p.m. (2017-04-21 15:19:05 UTC) #2
hub
On 2017/04/21 15:19:05, sergei wrote: > Could you please make constructor accepting JsValue&& as protected, ...
April 21, 2017, 3:46 p.m. (2017-04-21 15:46:38 UTC) #3
sergei
LGTM, though move constructor is absent
April 21, 2017, 5:51 p.m. (2017-04-21 17:51:22 UTC) #4
sergei
Could you please add move-ctrs? They are essential for performance, especially on android. Copy constructors ...
April 24, 2017, 8:18 a.m. (2017-04-24 08:18:30 UTC) #5
hub
On 2017/04/24 08:18:30, sergei wrote: > Could you please add move-ctrs? They are essential for ...
April 24, 2017, 1:45 p.m. (2017-04-24 13:45:59 UTC) #6
sergei
https://codereview.adblockplus.org/29419623/diff/29420587/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29419623/diff/29420587/include/AdblockPlus/FilterEngine.h#newcode130 include/AdblockPlus/FilterEngine.h:130: Subscription(Subscription&&) = default; On windows we are currently bound ...
April 24, 2017, 2:12 p.m. (2017-04-24 14:12:36 UTC) #7
hub
updated patch. https://codereview.adblockplus.org/29419623/diff/29420587/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29419623/diff/29420587/include/AdblockPlus/FilterEngine.h#newcode130 include/AdblockPlus/FilterEngine.h:130: Subscription(Subscription&&) = default; On 2017/04/24 14:12:36, sergei ...
April 24, 2017, 3:02 p.m. (2017-04-24 15:02:42 UTC) #8
sergei
https://codereview.adblockplus.org/29419623/diff/29420597/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29419623/diff/29420597/include/AdblockPlus/FilterEngine.h#newcode108 include/AdblockPlus/FilterEngine.h:108: Subscription& operator=(const Subscription&); The same comments as in https://codereview.adblockplus.org/29419629/#msg8
April 24, 2017, 6:19 p.m. (2017-04-24 18:19:36 UTC) #9
hub
updated patch. https://codereview.adblockplus.org/29419623/diff/29420597/include/AdblockPlus/FilterEngine.h File include/AdblockPlus/FilterEngine.h (right): https://codereview.adblockplus.org/29419623/diff/29420597/include/AdblockPlus/FilterEngine.h#newcode108 include/AdblockPlus/FilterEngine.h:108: Subscription& operator=(const Subscription&); On 2017/04/24 18:19:36, sergei ...
April 24, 2017, 7:16 p.m. (2017-04-24 19:16:51 UTC) #10
sergei
April 25, 2017, 7:38 a.m. (2017-04-25 07:38:26 UTC) #11
LGTM

Powered by Google App Engine
This is Rietveld