Issue 6378 - [emscripten] Make DependentString constexpr

compiled/String.h

 /*
  * 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 63 matching lines @@
 
 protected:
   value_type* mBuf;
   size_type mLen;
 
   constexpr explicit String(value_type* buf, size_type len, size_type flags)
       : mBuf(buf), mLen((len & LENGTH_MASK) | flags)
   {
   }
 
   ~String() = default;

[Eric, 2018/03/15 17:05:00]

We're not defining copy or move constructors, so there doesn't seem to be any
need to define a default destructor. Omitting this would make the destructor
public, but there's no downside to that.

[René Jeschke, 2018/03/15 17:41:23]

What was the reason to make the dtor protected in the beginning?

[Eric, 2018/03/15 18:33:54]

Don't know. If there's a good reason, it deserves a one-line comment. Sergei?
Hubert?

[sergei, 2018/03/16 13:43:26]

Such class hierarchy caused questions from the beginning but we agreed to go
with it for the sake of convenience and performance. However it's not supposed
that an instance of `String` can live without either `DependentString` or
`OwnedString`, so having of the protected destructor ensures that no one can
forget to call the destructor of `OwnedString`.

 
   void reset(value_type* buf, size_type len, size_type flags)
   {
     mBuf = buf;
     mLen = (len & LENGTH_MASK) | flags;
   }
 
 public:
   constexpr size_type length() const

[Eric, 2018/03/15 17:05:00]

constexpr implies const, so the trailing declaration can be dropped here and in
several places below.

[René Jeschke, 2018/03/15 17:41:23]

No, it does not. 'constexpr' on an object declaration implies 'const', not on a
function declaration.

[Eric, 2018/03/15 18:33:54]

Bah. Misread the spec. Never mind.

   {
     return mLen & LENGTH_MASK;
   }
 
   constexpr bool empty() const
   {
     return !(mLen & LENGTH_MASK);
   }
 
   constexpr const value_type* data() const
@@ skipping 16 matching lines @@
   {
     String_assert_writable(is_writable());
     return mBuf[pos];
   }
 
   constexpr bool is_writable() const
   {
     return (mLen & FLAGS_MASK) == READ_WRITE;
   }
 
-  bool equals(const String& other) const

[Eric, 2018/03/15 17:05:00]

I'd make this private, since its behavior is exposed through operator==.

[René Jeschke, 2018/03/15 17:41:22]

'equals' is already used in 'compiled/filter/RegExpFilter.cpp', so I won't make
it private.

[Eric, 2018/03/15 18:33:54]

OK. It would be better to change it there, but that's getting outside the scope
of the current change set.

[sergei, 2018/03/16 13:43:26]

Yes, it's outside the scope of this issue and there is no reason to make it
private. However we could perhaps get rid of `equals`, though it seems it does
not really harm. Please, create an issue for it if you think it is worth
discussing and let's continue there.

+  constexpr bool equals(const String& other) const
   {
     if (length() != other.length())
       return false;
 
     return std::memcmp(mBuf, other.mBuf, sizeof(value_type) * length()) == 0;
   }
 
-  bool operator==(const String& other) const

[Eric, 2018/03/15 17:05:01]

This can be constexpr, no?

[René Jeschke, 2018/03/15 17:41:23]

Yep, 'equals' and 'operator==' can both be 'constexpr'.

Done.

+  constexpr bool operator==(const String& other) const
   {
     return equals(other);
   }
 
-  bool operator!=(const String& other) const
+  constexpr bool operator!=(const String& other) const
   {
     return !equals(other);
   }
 
-  size_type find(value_type c, size_type pos = 0) const
+  constexpr size_type find(value_type c, size_type pos = 0) const
   {
     for (size_type i = pos; i < length(); ++i)
       if (mBuf[i] == c)
         return i;
     return npos;
   }
 
-  size_type find(const String& str, size_type pos = 0) const
+  constexpr size_type find(const String& str, size_type pos = 0) const
   {
     return find(str.mBuf, pos, str.length());
   }
 
-  size_type find(const value_type* str, size_type pos, size_type count) const
+  constexpr size_type find(const value_type* str, size_type pos, size_type count) const
   {
     if (pos > LENGTH_MASK || pos + count > length())
       return npos;
 
     if (!count)
       return pos;
 
     for (; pos + count <= length(); ++pos)
     {
       if (mBuf[pos] == str[0] &&
           std::memcmp(mBuf + pos, str, sizeof(value_type) * count) == 0)
       {
         return pos;
       }
     }
 
     return npos;
   }
 
-  size_type rfind(value_type c, size_type pos = npos) const
+  constexpr size_type rfind(value_type c, size_type pos = npos) const
   {
     if (length() == 0)
       return npos;
 
     if (pos >= length())
       pos = length() - 1;
 
     for (int i = pos; i >= 0; --i)
       if (mBuf[i] == c)
         return i;
@@ skipping 118 matching lines @@
   }
 };
 
 #ifdef INSIDE_TESTS
 inline std::ostream& operator<<(std::ostream& os, const DependentString& str)
 {
   return os << static_cast<const String&>(str);
 }
 #endif
 
-inline constexpr DependentString operator "" _str(const String::value_type* str,

[Eric, 2018/03/15 17:05:01]

constexpr on functions implies inline, so it could be dropped.

[René Jeschke, 2018/03/15 17:41:23]

Right. Done.

+constexpr DependentString operator "" _str(const String::value_type* str,
     String::size_type len)
 {
   return DependentString(str, len);
 }
 
 inline void String_assert_writable(bool isWritable)
 {
   assert2(isWritable, ABP_TEXT("Writing access to a read-only string"_str));
 }
 
@@ skipping 231 matching lines @@
   return OwnedString{value};
 }
 
 DependentString TrimSpaces(const String& value);
 
 // Splits the `value` string into two `DependentString`s excluding the character staying at `separatorPos`.
 // Useful for parsing.
 std::pair<DependentString, DependentString> SplitString(const String& value, String::size_type separatorPos);
 
 ABP_NS_END