Issue 5174 - Allow '{' and '}' in selectors

compiled/filter/ElemHideBase.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
+
+#include <cstring>
 
 #include "ElemHideBase.h"
 #include "../StringScanner.h"
 
 namespace
 {
   void NormalizeWhitespace(DependentString& text, String::size_type& domainsEnd,
       String::size_type& selectorStart)
   {
     // For element hiding filters we only want to remove spaces preceding the
@@ skipping 84 matching lines @@
 
   if (exception)
     return Type::ELEMHIDEEXCEPTION;
 
   if (text.find(u"[-abp-properties="_str, data.mSelectorStart) != text.npos)
     return Type::ELEMHIDEEMULATION;
 
   return Type::ELEMHIDE;
 }
 
-namespace {

[Wladimir Palant, 2017/10/18 09:24:02]

Please put the opening curly on the next line and indent the code inside this
block.

[hub, 2017/10/18 12:53:47]

Done.

+namespace
+{
+  static constexpr String::value_type OPENING_CURLY_REPLACEMENT[] = u"\\7B ";
+  static constexpr String::value_type CLOSING_CURLY_REPLACEMENT[] = u"\\7D ";
+  static constexpr String::size_type CURLY_REPLACEMENT_SIZE = sizeof(OPENING_CURLY_REPLACEMENT) / sizeof(OPENING_CURLY_REPLACEMENT[0]) - 1;
 
-static constexpr size_t REPLACE_SIZE = 5;

[Wladimir Palant, 2017/10/18 09:24:02]

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;

[hub, 2017/10/18 12:53:48]

Done.

+  OwnedString EscapeCurlies(String::size_type replacementCount,
+                            const DependentString& str)
+  {
+    OwnedString result(str.length() + replacementCount * (CURLY_REPLACEMENT_SIZE - 1));

[sergei, 2017/11/21 11:42:10]

I think we should not subtract 1 here because it's already done above.

[hub, 2017/11/21 15:58:21]

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.

[sergei, 2017/11/21 16:18:00]

Acknowledged.

 
-OwnedString EscapeCurlies(String::size_type count, const DependentString& str)
-{
-  OwnedString result(str.length() + (count * (REPLACE_SIZE - 1)));

[Wladimir Palant, 2017/10/18 09:24:02]

Nit: the parentheses around `count * ...` are unnecessary, multiplication is
evaluated before addition.

[hub, 2017/10/18 12:53:47]

Done.

-
-  String::size_type start = 0;
-  String::size_type i = 0;
-  String::value_type* current = result.data();
-  for (; i < str.length(); i++)
-  {
-    if (str[i] == '}' || str[i] == '{')
+    String::value_type* current = result.data();
+    for (String::size_type i = 0; i < str.length(); i++)
     {
-      if (i != start)
-      {
-        std::memcpy(current, str.data() + start,
-                    sizeof(String::value_type) * (i - start));
-        current += i - start;

[Wladimir Palant, 2017/10/18 09:24:02]

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];
  }

[hub, 2017/10/18 12:53:48]

Done.

-      }
-      start = i + 1;
       switch(str[i])
       {
-      case '}':

[Wladimir Palant, 2017/10/18 09:24:02]

Nit: I'd probably say u'}' to indicate that we are dealing with char16_t here.

[hub, 2017/10/18 12:53:47]

Done.

-        std::memcpy(current, u"\\x7D ",
-                    sizeof(String::value_type) * REPLACE_SIZE);

[Wladimir Palant, 2017/10/18 09:24:02]

You need to include <cstring> explicitly to use std::memcpy.

[hub, 2017/10/18 12:53:47]

Done.

-        current += REPLACE_SIZE;
+      case u'}':
+        std::memcpy(current, CLOSING_CURLY_REPLACEMENT,
+                    sizeof(String::value_type) * CURLY_REPLACEMENT_SIZE);
+        current += CURLY_REPLACEMENT_SIZE;
         break;
-      case '{':
-        std::memcpy(current, u"\\x7B ",
-                    sizeof(String::value_type) * REPLACE_SIZE);
-        current += REPLACE_SIZE;
+      case u'{':
+        std::memcpy(current, OPENING_CURLY_REPLACEMENT,
+                    sizeof(String::value_type) * CURLY_REPLACEMENT_SIZE);
+        current += CURLY_REPLACEMENT_SIZE;
         break;
       default:
+        *current = str[i];
+        current++;
         break;
       }
     }
+
+    return result;
   }
-  if (start < i)
-    std::memcpy(current, str.data() + start,
-                sizeof(String::value_type) * (i - start));
-  return result;
-}
-
 }
 
 OwnedString ElemHideBase::GetSelector() const
 {
   DependentString selector = mData.GetSelector(mText);
-  String::size_type count = 0;

[Wladimir Palant, 2017/10/18 09:24:02]

Nit: Somewhat less generic name such as replacementCount maybe?

[hub, 2017/10/18 12:53:47]

Done.

+  String::size_type replacementCount = 0;
   for (String::size_type i = 0; i < selector.length(); i++)
     if (selector[i] == '}' || selector[i] == '{')
-      count++;
-  if (count)
-    return EscapeCurlies(count, selector);
+      replacementCount++;
+  if (replacementCount)
+    return EscapeCurlies(replacementCount, selector);
 
   return OwnedString(selector);
 }
 
 OwnedString ElemHideBase::GetSelectorDomain() const
 {
   /* TODO this is inefficient */
   OwnedString result;
   if (mDomains)
   {
     for (const auto& item : *mDomains)
     {
       if (item.second && !item.first.empty())
       {
         if (!result.empty())
           result.append(u',');
         result.append(item.first);
       }
     }
   }
   return result;
 }