[PATCH] A new ADT: StringRefNulTerminated

Sean Silva silvas at purdue.edu
Wed Feb 20 16:34:21 PST 2013


Overall, I think this is a good idea to have.

Some comments about the patch:

+    /// Construct a string ref from a string literal.
+    template<size_t LengthWithNul>
+    StringRefNulTerminated(const char (&Str)[LengthWithNul]):
+      StringRef(Str, LengthWithNul - 1) {}

Is there a threat that this could end up being called by a non-string
literal? If so, then it should assert. I'm thinking of a scenario like

const char Foo[1] = {'a'};
auto X = StringRefNulTerminated(Foo);

+  /// \brief Represents a constant reference to a NUL-terminated string.
+  /// It can be safely converted to 'const char *' without copying the
+  /// underlying data.
+  class StringRefNulTerminated : public StringRef {
+  public:

This comment should really say something explaining that "it is just a
way to represent in the type system that 'one past the end' of the
StringRef is valid memory guaranteed to be NUL". The discussion in the
rst documentation could be significantly simplified by describing it
like this as well.

+  /// It can be safely converted to 'const char *' without copying the
+  /// underlying data.

You should make clear that this is the motivation e.g. "The motivation
is that it can be safely ...". I would also drop "safely" to avoid
even suggesting the idea of looking one-past-the-end of a regular
StringRef.

+    StringRefNulTerminated(const char *Data, size_t Length):
+        StringRef(Data, Length) {
+      assert(Data[Length] == '\0' && "");

Please make the message actually helpful. When someone crashes on this
assert they want something meaningful.

-- Sean Silva



More information about the llvm-commits mailing list