Issue 6378 - [emscripten] Make DependentString constexpr

test/compiled/String.cpp

Index: test/compiled/String.cpp
diff --git a/test/compiled/String.cpp b/test/compiled/String.cpp
index e969ccde556eb61d9b3b67a6303fca1eddec8b9b..54f18ad668e89c1dd2cc075b78d0b2aa409e9837 100644
--- a/test/compiled/String.cpp
+++ b/test/compiled/String.cpp
@@ -21,6 +21,25 @@
 
 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");
+
+  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[1] == 'e', "Second character should be 'e'");
+
+  static_assert(s2.is_invalid(), "String should be invalid");
+}
+
 TEST(TestString, constructInvalidDependentString)
 {
   DependentString s;