Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(751)

Issue 29548581: Issue 4128, 5138 - Add Parser and Serializer implemented in C++

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 3 weeks ago by sergei
Modified:
1 week, 6 days ago
Reviewers:
hub, Wladimir Palant
CC:
Felix Dahlke, tlucas
Base URL:
https://github.com/adblockplus/adblockpluscore.git
Visibility:
Public.

Description

- move the check whether a template parameter of `intrusive_ptr` inherits `ref_counted` in `static_assert` in the class definition in order to use it with yet incomplete class, e.g. `class Subscription; typedef intrusive_ptr<Subscription> SubscriptionPtr;` - add `addRef = true` argument to `intrusive_ptr::reset`, so it has the same functionality as the constructor. - add `String::toInt64` and `String::toUInt64` parsing the string into an integer - add a very basic `template<typename Type> Type AdaptValue(const String& value)` function allowing to easily convert a string value into a desired `Type`. - add `DependentString TrimSpaces(const String& value)` removing ASCII space characters on both sides of the argument - add `std::pair<DependentString, DependentString> SplitString(const String& value, String::size_type separatorPos)` splitting the string into two parts without a separator character - don't expose in JS API `Subscription::Serialize` - remove `Subscription::SerializeFilters` - add JS _FilterStorage_Parser bound to C++ Parser and is aliased to require("filterStorage").Parser. - add JS _FilterStorage_Serializer bound to C++ `Serializer` and is aliased to require("filterStorage").Serializer - add C++ only `Subscription` factory method `Subscription::FromProperties(const KeyValues& properties)`, `Subscription::KeyValues` and constructor for `Subscription` classes accepting `KeyValues` allowing to construct a subscription from pairs of subscription properties according to the `url` property (either `DownloadableSubscritpion` or `UserDefinedSubscription`). - make the constructor of `Subscription` protected - add C++ only version of `Subscription::AddFilter(Filter&)` adding a filter to the end of subscription filters without notifications. - add C++ only `const Filters& Subscription::GetFilters() const`, for present it's used by `Serializer`. - rename `Subscription::Serialize` to `Subscription::SerializerProperties` because it serializes only certain subscription properties. It's kept as a member of `Subscription` for the sake of simplicity. - add the const version of `Subscription::As()` - add test/_test-utils.js with `withNAD` reducing the amount of code working with objects from emscripten and with `testEqualObjProperties` comparing properties of emscripten classes objects. - update tests The implementations of both `Parser` and `Serializer` are not complete yet in accordance with the issue. It is the first step having the core functionality, allowing to write tests and start to use them, at least partially. In particular, beside some other things, `Parser` is not connected with `FilterStroage` yet and it has to implicitly affect global `knownFilters` and `knownSubscriptions`, `Serializer` does not provide with generator-like interface, does not do anything keeping a passed subscription unmodified externally during serialization process and is not connected with `FilterEngine` either, and there is no conversion to UTF-8.

Patch Set 1 #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+819 lines, -108 lines) Patch
M compiled/String.h View 3 chunks +66 lines, -0 lines 3 comments Download
A compiled/String.cpp View 1 chunk +32 lines, -0 lines 0 comments Download
M compiled/bindings/main.cpp View 5 chunks +16 lines, -6 lines 0 comments Download
M compiled/intrusive_ptr.h View 2 chunks +4 lines, -4 lines 0 comments Download
A compiled/storage/Parser.h View 1 chunk +65 lines, -0 lines 0 comments Download
A compiled/storage/Parser.cpp View 1 chunk +183 lines, -0 lines 0 comments Download
A compiled/storage/Serializer.h View 1 chunk +40 lines, -0 lines 0 comments Download
A compiled/storage/Serializer.cpp View 1 chunk +72 lines, -0 lines 1 comment Download
M compiled/subscription/DownloadableSubscription.h View 2 chunks +2 lines, -2 lines 0 comments Download
M compiled/subscription/DownloadableSubscription.cpp View 1 chunk +18 lines, -5 lines 1 comment Download
M compiled/subscription/Subscription.h View 3 chunks +37 lines, -7 lines 0 comments Download
M compiled/subscription/Subscription.cpp View 5 chunks +45 lines, -29 lines 0 comments Download
M compiled/subscription/UserDefinedSubscription.h View 2 chunks +4 lines, -2 lines 0 comments Download
M compiled/subscription/UserDefinedSubscription.cpp View 3 chunks +25 lines, -4 lines 0 comments Download
M lib/filterStorage.js View 2 chunks +4 lines, -1 line 0 comments Download
A test/_test-utils.js View 1 chunk +81 lines, -0 lines 0 comments Download
M test/subscriptionClasses.js View 6 chunks +125 lines, -48 lines 0 comments Download

Messages

Total messages: 4
sergei
https://codereview.adblockplus.org/29548581/diff/29548701/compiled/subscription/DownloadableSubscription.cpp File compiled/subscription/DownloadableSubscription.cpp (left): https://codereview.adblockplus.org/29548581/diff/29548701/compiled/subscription/DownloadableSubscription.cpp#oldcode25 compiled/subscription/DownloadableSubscription.cpp:25: SetTitle(id); Why is the title set to ID only ...
2 months, 3 weeks ago (2017-09-18 19:20:07 UTC) #1
sergei
https://codereview.adblockplus.org/29548581/diff/29548701/compiled/storage/Serializer.cpp File compiled/storage/Serializer.cpp (right): https://codereview.adblockplus.org/29548581/diff/29548701/compiled/storage/Serializer.cpp#newcode67 compiled/storage/Serializer.cpp:67: auto escapedResult = EscapeOpeningBracket(filter->GetText()); BTW, what about escaping of ...
2 months, 3 weeks ago (2017-09-19 09:17:35 UTC) #2
hub
https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.h File compiled/String.h (right): https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.h#newcode212 compiled/String.h:212: } No overflow checking? int64_t is quite a lot ...
3 weeks, 1 day ago (2017-11-22 15:40:13 UTC) #3
hub
1 week, 6 days ago (2017-11-30 21:35:37 UTC) #4
https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.h
File compiled/String.h (right):

https://codereview.adblockplus.org/29548581/diff/29548701/compiled/String.h#n...
compiled/String.h:212: }
On 2017/11/22 15:40:12, hub wrote:
> No overflow checking?
> 
> int64_t is quite a lot of significant digits, but we should fail parsing if
> going too far. On the 19th we should bail. (20th for uint64_t)
> 

std::numeric_limits<int64_t>::digit10 is the value you are looking for. And you
could combine toInt64() and toUInt64() with templates and use
std::numeric_limits<T>::is_signed() to reject when the value is unsigned.

I have my own version of this in https://codereview.adblockplus.org/29606600/ -
we should combine them. I don't mind actually making a separate patch that we
could review and land prior any of our reviews.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5