Issue 29580558: Issue 5174 - Allow '{' and '}' in selectors (Closed)
|
Created:
Oct. 16, 2017, 5:10 p.m. by hub Modified:
Nov. 21, 2017, 5 p.m. Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 5174 - Allow '{' and '}' in selectors
Patch Set 1 #Patch Set 2 : Fix a bug. Elememulation test case. #
Total comments: 5
Patch Set 3 : Preallocate the destination #
Total comments: 14
Patch Set 4 : adressed comments #
Total comments: 2
Patch Set 5 : Use the proper escape sequence #
Total comments: 2
Patch Set 6 : Indent namespace content. #
Total comments: 3
MessagesTotal messages: 15
https://codereview.adblockplus.org/29580558/diff/29580599/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29580558/diff/29580599/test/filterClasses.... test/filterClasses.js:391: filter = Filter.fromText("foo.com##[-abp-properties='/margin: [3-4]{2}/']"); we don't handle the new syntax yet.
https://codereview.adblockplus.org/29580558/diff/29580599/compiled/filter/Ele... File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29580599/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:150: result.append(str.data() + start, i - start); This is performing lots of reallocations. Please calculate the string size up front and allocate the buffer as necessary. https://codereview.adblockplus.org/29580558/diff/29580599/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29580558/diff/29580599/test/filterClasses.... test/filterClasses.js:391: filter = Filter.fromText("foo.com##[-abp-properties='/margin: [3-4]{2}/']"); On 2017/10/16 18:39:43, hub wrote: > we don't handle the new syntax yet. Feel free to file an issue for that.
https://codereview.adblockplus.org/29580558/diff/29580599/compiled/filter/Ele... File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29580599/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:150: result.append(str.data() + start, i - start); On 2017/10/17 10:24:33, Wladimir Palant wrote: > This is performing lots of reallocations. Please calculate the string size up > front and allocate the buffer as necessary. Done. https://codereview.adblockplus.org/29580558/diff/29580599/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29580558/diff/29580599/test/filterClasses.... test/filterClasses.js:391: filter = Filter.fromText("foo.com##[-abp-properties='/margin: [3-4]{2}/']"); On 2017/10/17 10:24:33, Wladimir Palant wrote: > On 2017/10/16 18:39:43, hub wrote: > > we don't handle the new syntax yet. > > Feel free to file an issue for that. Filed https://issues.adblockplus.org/ticket/5870
https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/Ele... File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:121: namespace { Please put the opening curly on the next line and indent the code inside this block. https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:123: static constexpr size_t REPLACE_SIZE = 5; This name doesn't quite make it clear what this constant is about. CURLY_REPLACEMENT_SIZE maybe? Also, it would be nice if we wouldn't hardcode the value. Maybe something like: static constexpr String::value_type[] OPENING_CURLY_REPLACEMENT = u"\\x7B "; static constexpr String::value_type[] CLOSING_CURLY_REPLACEMENT = u"\\x7D "; static constexpt size_t CURLY_REPLACEMENT_SIZE = sizeof(OPENING_CURLY_REPLACEMENT) / sizeof(OPENING_CURLY_REPLACEMENT[0]) - 1; https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:127: OwnedString result(str.length() + (count * (REPLACE_SIZE - 1))); Nit: the parentheses around `count * ...` are unnecessary, multiplication is evaluated before addition. https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:140: current += i - start; The logic here got unnecessarily complicated. I'd rather go with: for (String::size_type i = 0, j = 0; i < str.length(); i++, j++) { if (str[i] == u'{') ... else if (str[i] == u'}') ... else result[j] = str[i]; } https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:145: case '}': Nit: I'd probably say u'}' to indicate that we are dealing with char16_t here. https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:147: sizeof(String::value_type) * REPLACE_SIZE); You need to include <cstring> explicitly to use std::memcpy. https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:171: String::size_type count = 0; Nit: Somewhat less generic name such as replacementCount maybe?
https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/Ele... File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:121: namespace { On 2017/10/18 09:24:02, Wladimir Palant wrote: > Please put the opening curly on the next line and indent the code inside this > block. Done. https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:123: static constexpr size_t REPLACE_SIZE = 5; On 2017/10/18 09:24:02, Wladimir Palant wrote: > This name doesn't quite make it clear what this constant is about. > CURLY_REPLACEMENT_SIZE maybe? > > Also, it would be nice if we wouldn't hardcode the value. Maybe something like: > > static constexpr String::value_type[] OPENING_CURLY_REPLACEMENT = u"\\x7B "; > static constexpr String::value_type[] CLOSING_CURLY_REPLACEMENT = u"\\x7D "; > static constexpt size_t CURLY_REPLACEMENT_SIZE = > sizeof(OPENING_CURLY_REPLACEMENT) / sizeof(OPENING_CURLY_REPLACEMENT[0]) - 1; Done. https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:127: OwnedString result(str.length() + (count * (REPLACE_SIZE - 1))); On 2017/10/18 09:24:02, Wladimir Palant wrote: > Nit: the parentheses around `count * ...` are unnecessary, multiplication is > evaluated before addition. Done. https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:140: current += i - start; On 2017/10/18 09:24:02, Wladimir Palant wrote: > The logic here got unnecessarily complicated. I'd rather go with: > > for (String::size_type i = 0, j = 0; i < str.length(); i++, j++) > { > if (str[i] == u'{') > ... > else if (str[i] == u'}') > ... > else > result[j] = str[i]; > } Done. https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:145: case '}': On 2017/10/18 09:24:02, Wladimir Palant wrote: > Nit: I'd probably say u'}' to indicate that we are dealing with char16_t here. Done. https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:147: sizeof(String::value_type) * REPLACE_SIZE); On 2017/10/18 09:24:02, Wladimir Palant wrote: > You need to include <cstring> explicitly to use std::memcpy. Done. https://codereview.adblockplus.org/29580558/diff/29581906/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:171: String::size_type count = 0; On 2017/10/18 09:24:02, Wladimir Palant wrote: > Nit: Somewhat less generic name such as replacementCount maybe? Done.
LGTM https://codereview.adblockplus.org/29580558/diff/29582624/compiled/filter/Ele... File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29582624/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:128: static constexpr String::size_type CURLY_REPLACEMENT_SIZE = sizeof(OPENING_CURLY_REPLACEMENT) / sizeof(OPENING_CURLY_REPLACEMENT[0]) - 1; What about a constexpr function calculating the length of the string instead of a global variable? It will be the same but resistant to any change of the strings, which is pretty unlikely though. https://codereview.adblockplus.org/29580558/diff/29582624/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:143: current += CURLY_REPLACEMENT_SIZE; What about moving of memcpy into the String class? If we do it, I think it should be rather in another commit. Despite it can introduce some overhead it can be very useful for other cases. String::size_type currentPos = 0; ... currentPos += result.write_at(currentPos, CLOSING_CURLY_REPLACEMENT, CURLY_REPLACEMENT_SIZE); // returns the number of written chars size_type String::write_at(size_type pos, const value_type* data, size_type count) { count = ... ; // ensure that it does not go beyond the length if (mBuf != nullptr && data != nullptr && count > 0) std::memcpy(current, data, sizeof(value_type) * count); return count; }
Fixed following. https://issues.adblockplus.org/ticket/5174#comment
On 2017/10/18 18:38:34, hub wrote: > Fixed following. > > https://issues.adblockplus.org/ticket/5174#comment I meant https://issues.adblockplus.org/ticket/5174#comment:7
LGTM
https://codereview.adblockplus.org/29580558/diff/29582685/compiled/filter/Ele... File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29582685/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:125: Nit: This newline is unnecessary. And the block contents still need to be indented.
https://codereview.adblockplus.org/29580558/diff/29582685/compiled/filter/Ele... File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29582685/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:125: On 2017/10/19 07:59:28, Wladimir Palant wrote: > Nit: This newline is unnecessary. And the block contents still need to be > indented. Done.
https://codereview.adblockplus.org/29580558/diff/29583579/compiled/filter/Ele... File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29583579/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:132: OwnedString result(str.length() + replacementCount * (CURLY_REPLACEMENT_SIZE - 1)); I think we should not subtract 1 here because it's already done above.
https://codereview.adblockplus.org/29580558/diff/29583579/compiled/filter/Ele... File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29583579/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:132: OwnedString result(str.length() + replacementCount * (CURLY_REPLACEMENT_SIZE - 1)); On 2017/11/21 11:42:10, sergei wrote: > I think we should not subtract 1 here because it's already done above. the -1 is because we are replacing. str.length() already counts for the '{' or '}' as CURLY_REPLACEMENT_SIZE is the length of the string (sizeof() returns byte size with the terminating 0 hence the -1) which is 4. So we end up just adding 3 per replacement.
LGTM https://codereview.adblockplus.org/29580558/diff/29583579/compiled/filter/Ele... File compiled/filter/ElemHideBase.cpp (right): https://codereview.adblockplus.org/29580558/diff/29583579/compiled/filter/Ele... compiled/filter/ElemHideBase.cpp:132: OwnedString result(str.length() + replacementCount * (CURLY_REPLACEMENT_SIZE - 1)); On 2017/11/21 15:58:21, hub wrote: > On 2017/11/21 11:42:10, sergei wrote: > > I think we should not subtract 1 here because it's already done above. > > the -1 is because we are replacing. str.length() already counts for the '{' or > '}' as CURLY_REPLACEMENT_SIZE is the length of the string (sizeof() returns byte > size with the terminating 0 hence the -1) which is 4. So we end up just adding 3 > per replacement. Acknowledged. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
