[PATCH] A new ADT: StringRefNulTerminated

[David Blaikie]

On Wed, Feb 20, 2013 at 4:34 PM, Sean Silva <silvas at purdue.edu> wrote:

> 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
>

As much fun as templated array ref ctors are - it's probably simpler just
to take a const char*, use strlen always (you know it's null terminated -
or at least the user is in the wrong if they pass you one that isn't) & let
the compiler optimize away the strlen/inline the call when it's convenient
to do so.

That would catch cases of large buffers with small null terminated strings
in them which the current patch doesn't handle correctly.


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

Like any c-string API, that's just UB. Sure, I suppose we could use the
array-ref style thing to detect & assert on this, might be handy - but we
don't do it for other cstring APIs so I don't see any particular reason to
do it on this one.


>
> +  /// \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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130220/06b6a0c1/attachment.html>