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
@@ skipping 104 matching lines @@
     return Type::ELEMHIDEEXCEPTION;
 
   if (text.find(u"[-abp-properties="_str, data.mSelectorStart) != text.npos)
     return Type::ELEMHIDEEMULATION;
 
   return Type::ELEMHIDE;
 }
 
 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 String::value_type OPENING_CURLY_REPLACEMENT[] = u"\\x7B ";
-static constexpr String::value_type CLOSING_CURLY_REPLACEMENT[] = u"\\x7D ";
-static constexpr String::size_type CURLY_REPLACEMENT_SIZE = sizeof(OPENING_CURLY_REPLACEMENT) / sizeof(OPENING_CURLY_REPLACEMENT[0]) - 1;

[sergei, 2017/10/18 14:01:37]

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.

+  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 replacementCount,
-                          const DependentString& str)
-{
-  OwnedString result(str.length() + replacementCount * (CURLY_REPLACEMENT_SIZE - 1));
+    String::value_type* current = result.data();
+    for (String::size_type i = 0; i < str.length(); i++)
+    {
+      switch(str[i])
+      {
+      case u'}':
+        std::memcpy(current, CLOSING_CURLY_REPLACEMENT,
+                    sizeof(String::value_type) * CURLY_REPLACEMENT_SIZE);
+        current += CURLY_REPLACEMENT_SIZE;
+        break;
+      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;
+      }
+    }
 
-  String::value_type* current = result.data();
-  for (String::size_type i = 0; i < str.length(); i++)
-  {
-    switch(str[i])
-    {
-    case u'}':
-      std::memcpy(current, CLOSING_CURLY_REPLACEMENT,
-                  sizeof(String::value_type) * CURLY_REPLACEMENT_SIZE);
-      current += CURLY_REPLACEMENT_SIZE;

[sergei, 2017/10/18 14:01:37]

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

-      break;
-    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;
   }
-
-  return result;
-}
-
 }
 
 OwnedString ElemHideBase::GetSelector() const
 {
   DependentString selector = mData.GetSelector(mText);
   String::size_type replacementCount = 0;
   for (String::size_type i = 0; i < selector.length(); i++)
     if (selector[i] == '}' || selector[i] == '{')
       replacementCount++;
   if (replacementCount)
@@ skipping 13 matching lines @@
       if (item.second && !item.first.empty())
       {
         if (!result.empty())
           result.append(u',');
         result.append(item.first);
       }
     }
   }
   return result;
 }