Bug 1405582 - Span: delete implicit constructors for char* and char16…

…_t*. r=froydnj,hsivonen

Delete Span's implicit constructors for char* and char16_t* pointers to avoid accidental construction in cases where a pointer does not point to a zero-terminated string. Use the MakeStringSpan() function instead.

I deleted both the const and non-const char* and char16_t* constructors, in the name of cross-compiler consistency. If we only delete the const char* and char16_t* constructors, for some reason, MSVC complains that `Span<char> s(charArray)` uses a deleted constructor while clang nor gcc permit it. I don't know if this is a compiler bug in MSVC or clang and gcc.

Also, do not permit MakeSpan() for string literals (const char and char16_t arrays) because the Span length would include the zero terminator, which may surprise callers. Use MakeStringSpan() to create a Span whose length that excludes the string literal's zero terminator or use the MakeSpan() overload that accepts a pointer and length and specify the string literal's full length.

The following Span usages are prevented:

  Span<const char> span("literal"); // error
  Span<char> span(charArray); // error

  Span<const char> span;
  span = "literal"; // error
  span = charArray; // error

  MakeSpan("literal"); // error

The following Span usages are still permitted:

  assert(MakeStringSpan("literal") == 8); // OK: span length is calculated with strlen() and excludes the zero terminator
  MakeStringSpan(charArray); // OK: span length is calculated with strlen() and excludes the zero terminator
  MakeSpan(charArray); // OK: span length is the char array size including any zero terminator

MozReview-Commit-ID: Et71CpjsiyI

mfbt/Span.h

@@ -500,6 +500,16 @@ class Span
   {
   }
 
+  // Implicit constructors for char* and char16_t* pointers are deleted in order
+  // to avoid accidental construction in cases where a pointer does not point to
+  // a zero-terminated string. A Span<const char> or Span<const char16_t> can be
+  // obtained for const char* or const char16_t pointing to a zero-terminated
+  // string using the MakeStringSpan() function.
+  Span(char* aStr) = delete;
+  Span(const char* aStr) = delete;
+  Span(char16_t* aStr) = delete;
+  Span(const char16_t* aStr) = delete;
+
   /**
    * Constructor for std::array.
    */
@@ -971,11 +981,20 @@ MakeSpan(ElementType* aStartPtr, ElementType* aEndPtr)
 
 /**
  * Create span from C array.
+ * MakeSpan() does not permit creating Span objects from string literals (const
+ * char or char16_t arrays) because the Span length would include the zero
+ * terminator, which may surprise callers. Use MakeStringSpan() to create a
+ * Span whose length that excludes the string literal's zero terminator or use
+ * the MakeSpan() overload that accepts a pointer and length and specify the
+ * string literal's full length.
  */
-template<class ElementType, size_t N>
+template<class ElementType, size_t N,
+         class = span_details::enable_if_t<
+                   !IsSame<ElementType, const char>::value &&
+                   !IsSame<ElementType, const char16_t>::value>>
 Span<ElementType> MakeSpan(ElementType (&aArr)[N])
 {
-  return Span<ElementType>(aArr);
+  return Span<ElementType>(aArr, N);
 }
 
 /**

mfbt/tests/gtest/TestSpan.cpp

@@ -1201,6 +1201,37 @@ SPAN_TEST(from_cstring)
     ASSERT_EQ(cs.size(), 3U);
     ASSERT_EQ(cs.data(), str);
     ASSERT_EQ(cs[2], 'c');
+
+#ifdef CONFIRM_COMPILATION_ERRORS
+    Span<const char> scccl("literal"); // error
+
+    Span<const char> sccel;
+    sccel = "literal"; // error
+
+    cs = MakeSpan("literal"); // error
+#endif
+  }
+  {
+    char arr[4] = {'a', 'b', 'c', 0};
+
+    auto cs = MakeStringSpan(arr);
+    ASSERT_EQ(cs.size(), 3U);
+    ASSERT_EQ(cs.data(), arr);
+    ASSERT_EQ(cs[2], 'c');
+
+    cs = MakeSpan(arr);
+    ASSERT_EQ(cs.size(), 4U); // zero terminator is part of the array span.
+    ASSERT_EQ(cs.data(), arr);
+    ASSERT_EQ(cs[2], 'c');
+    ASSERT_EQ(cs[3], '\0'); // zero terminator is part of the array span.
+
+#ifdef CONFIRM_COMPILATION_ERRORS
+    Span<char> scca(arr); // error
+    Span<const char> sccca(arr); // error
+
+    Span<const char> scccea;
+    scccea = arr; // error
+#endif
   }
   {
     char16_t arr[4] = {'a', 'b', 'c', 0};
@@ -1210,6 +1241,31 @@ SPAN_TEST(from_cstring)
     ASSERT_EQ(cs.size(), 3U);
     ASSERT_EQ(cs.data(), str);
     ASSERT_EQ(cs[2], 'c');
+
+    cs = MakeStringSpan(arr);
+    ASSERT_EQ(cs.size(), 3U);
+    ASSERT_EQ(cs.data(), str);
+    ASSERT_EQ(cs[2], 'c');
+
+    cs = MakeSpan(arr);
+    ASSERT_EQ(cs.size(), 4U); // zero terminator is part of the array span.
+    ASSERT_EQ(cs.data(), str);
+    ASSERT_EQ(cs[2], 'c');
+    ASSERT_EQ(cs[3], '\0'); // zero terminator is part of the array span.
+
+#ifdef CONFIRM_COMPILATION_ERRORS
+    Span<char16_t> scca(arr); // error
+
+    Span<const char16_t> scccea;
+    scccea = arr; // error
+
+    Span<const char16_t> scccl(u"literal"); // error
+
+    Span<const char16_t> *sccel;
+    *sccel = u"literal"; // error
+
+    cs = MakeSpan(u"literal"); // error
+#endif
   }
 }