[llvm-commits] [PATCH] StringRef class.

[Daniel Dunbar]

On Mon, Jul 20, 2009 at 11:36 PM, Chris Lattner<clattner at apple.com> wrote:
> +  /// StringRef - Represent a constant reference to a string, i.e. a
> character
> +  /// array and a length.
> +  ///
> +  /// This class does not own the string data, it is expected to be
> used in
> +  /// situations where the character data resides in some other
> buffer, whose
> +  /// lifetime extends past that of the StringRef. For this reason,
> it is not in
> +  /// general safe to store a StringRef.
>
> Please also mention that the stringref is not necessarily nul-
> terminated.

Good point, added here and to the data() method.

> +    /// Construct a string ref from a cstring.
> +    /*implicit*/ StringRef(const char *Str)
> +      : Data(Str), Length(::strlen(Str)) {}
>
> No magic array template version that avoids strlen? :)

No, I don't think its possible to do this and have a method which
takes a const char*.

>
> Instead of:
>
> +    const char *begin() const { return Data; }
> +    const char *end() const { return Data + Length; }
>
> How about:
>
> +    iterator begin() const { return Data; }
> +    iterator end() const { return Data + Length; }

Yup!

> +    /// size - Get the string size.
> +    unsigned size() const { return Length; }
> +
> +    /// length - Get the string size.
> +    unsigned length() const { return Length; }
>
> I don't see a reason to have length(), lets just go with size(), which
> is more standard.

Okay, removed. I added length just to match std::string.

> +    int compare(const StringRef &RHS) const {
> +      // Check the prefix for a mismatch.
> +      if (int Res = memcmp(Data, RHS.Data, std::min(Length,
> RHS.Length)))
> +        return Res;
>
> Is memcmp guaranteed to return -1/0/1?  I thought it was just
> "negative, 0, positive"?  This will foul up your operator<.

Nice catch, of course I wrote my unit test with an ordinal difference
of 1. Fixed to honor the comment; although we could also just follow
memcmp semantics.

> +    bool operator==(const StringRef &RHS) const { return compare(RHS)
> == 0; }
> +    bool operator!=(const StringRef &RHS) const { return
> compare(RHS) != 0; }
>
> These are expensive equality checks.  For equality (but not ordering),
> you can do a quick reject based on the string length.

Another nice catch, fixed.

> +    bool operator<(const StringRef &RHS) const { return compare(RHS)
> == -1; }
>
> No >, <=, etc?

Added; I wasn't sure if we had a policy on this.

Thanks,
 - Daniel