Issue 6378 - [emscripten] Make DependentString constexpr

test/compiled/String.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 <string>
 #include <gtest/gtest.h>
 #include "compiled/String.h"
 
 ABP_NS_USING
 
-TEST(TestString, checkConstexprCorrectness)

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

Since all the static_assert statements don't generate runtime code, there's no
particular need to have this function be in a test suite. An ordinary void
function would work just as well.

I'd also separate these tests into three functions, one for each of the
constexpr constructors. Also, the s3 expression is (implicitly) combining a test
for one of these constructors with one for _str(), which deserves to be tested
separately in a fourth function.

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

Is it really worth it to clutter the test code with _four_ functions not
containing any code but static_asserts only?

Also, testing ctors will result in functions only having one variable definition
... which then would need to include a dummy test to prevent 'unused variable'
warnings.

The 's3' test case is fixed now, though, and tests for 'equals' and '==' are
included. Also I've made the function a non-test one.

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

I should preface these comments by saying that I always seem to test code far
more exhaustively than most. Lots of people think it's overkill; I don't.

On 2018/03/15 17:41:23, René Jeschke wrote:
In my experience, it would be. One main goal of unit testing to be able to
isolate faults as quickly as possible. Splitting up the tests by the particular
function/constructor that the test arises in speeds up that process. I wouldn't
call it clutter, either. The names of these functions provide documentation
about which specific thing is being tested.

The real value of this isn't now, when the code is still fresh, but later, when
it's being rewritten. It's then that all the testing overhead really comes into
its own. It's cheap to write the simple tests about assumptions; it's much
lengthier to reverse-engineer them later, particularly when it's someone else
who's having their first encounter.
Are you getting such warnings now? Take out the static_assert statements, which
only run at compile time, and all you've got left are constructors and
initialization expressions.

Depends on the compiler, but simple instantiation of a non-POD class doesn't
generally give that warning (presumably for possible constructor side-effects).
The first test I usually write on a class is just a default-constructed instance
(if appropriate) with no other statements. (The test is that the constructor
doesn't throw.) I don't see such unused variable messages.

[René Jeschke, 2018/03/15 19:02:34]

Okay, fine with me. I separated the tests. The only warning I got was for 's =
u"..."_str', so I used this one for function/operator tests.

-{
-  static constexpr DependentString s(u"Hello world!");
-  static constexpr DependentString s2;
-  static constexpr DependentString s3 = u"Hello again!"_str;
-
-  static_assert(s.length() == 12, "Length should be 12");
-  static_assert(s3.length() == 12, "Length should be 12");
+constexpr void checkConstexprTemplateCtor()
+{
+  constexpr DependentString s(u"Test string");
+}
+
+constexpr void checkConstexprInvalidCtor()
+{
+  constexpr DependentString s;
+  static_assert(s.is_invalid(), "String should be invalid");
+}
+
+constexpr void checkConstexprCtor()
+{
+  constexpr DependentString s(u"Test string", 11);
+}
+
+constexpr void checkConstexprFuncsAndOps()
+{
+  constexpr DependentString s = u"Test string"_str;
+  constexpr DependentString s2 = u"Test string"_str;
+  constexpr DependentString s3 = u"Another test string"_str;
+
+  static_assert(s.length() == s2.length(), "Length should be equal");
+  static_assert(s.equals(s2), "Strings should be equal");
+  static_assert(s == s2, "Strings should be equal");
+  static_assert(s != s3, "Strings should not be equal");
 
   static_assert(!s.empty(), "String should not be empty");
   static_assert(!s.is_writable(), "String should not be writable");
   static_assert(!s.is_deleted(), "String should not be deleted");
 
-  static_assert(s.data()[0] == 'H', "First character should be 'H'");
+  static_assert(s.data()[0] == 'T', "First character should be 'T'");
   static_assert(s[1] == 'e', "Second character should be 'e'");
 
-  static_assert(s2.is_invalid(), "String should be invalid");
+  static_assert(s.find(u'e') == 1, "'e' should be at pos 1");
+  static_assert(s.find(s2) == 0, "String should be found at pos 0");
+  static_assert(s.rfind(u'e') == 1, "'e' should be at pos 1");
 }
 
 TEST(TestString, constructInvalidDependentString)
 {
   DependentString s;
   EXPECT_TRUE(s.is_invalid());
 
   DependentString s2(s);
   EXPECT_TRUE(s2.is_invalid());
 }
@@ skipping 136 matching lines @@
   EXPECT_FALSE(lexical_cast<bool>(ABP_TEXT("some-string"_str)));
   EXPECT_FALSE(lexical_cast<bool>(ABP_TEXT(""_str)));
   EXPECT_FALSE(lexical_cast<bool>(DependentString()));
 }
 
 TEST(TestStringLexicalCast, toOwnedString)
 {
   EXPECT_EQ(ABP_TEXT("some-string"_str), lexical_cast<OwnedString>(ABP_TEXT("some-string"_str)));
   EXPECT_EQ(ABP_TEXT(""_str), lexical_cast<OwnedString>(ABP_TEXT(""_str)));
 }