[llvm-commits] [PATCH][Support/StringRef] Add rtrim.

[Michael Spencer]

On Fri, May 11, 2012 at 1:24 PM, Chandler Carruth <chandlerc at google.com> wrote:
> Minor comments below. Feel free to commit once addressed (even if that is
> "no").
>
> diff --git a/include/llvm/ADT/StringRef.h b/include/llvm/ADT/StringRef.h
> index 76ba66e..b1f376b 100644
> --- a/include/llvm/ADT/StringRef.h
> +++ b/include/llvm/ADT/StringRef.h
> @@ -292,6 +292,16 @@ namespace llvm {
>      /// Note: O(size() + Chars.size())
>      size_type find_last_of(StringRef Chars, size_t From = npos) const;
>
> +    /// find_last_not_of - Find the last character in the string that is
> not
> +    /// \arg C, or npos if not found.
> +    size_type find_last_not_of(char C, size_t From = npos) const ;
>
> extraneous space before the ';'.
>
> +
> +    /// find_last_not_of - Find the last character in the string that is
> not in
> +    /// \arg Chars, or npos if not found.
> +    ///
> +    /// Note: O(size() + Chars.size())
> +    size_type find_last_not_of(StringRef Chars, size_t From = npos) const ;
>
> extraneous space before the ';'.
>
> +
>      /// @}
>      /// @name Helpful Algorithms
>      /// @{
> @@ -480,6 +490,18 @@ namespace llvm {
>        return std::make_pair(slice(0, Idx), slice(Idx+1, npos));
>      }
>
> +    StringRef ltrim(StringRef Chars) const {
> +      return drop_front(std::min(Length, find_first_not_of(Chars)));
>
> find_first_not_of supports a single 'char' input, but this doesn't -- any
> particular reason?

No use case, but it's easy to add.

> Do we want to provide a whitespace trimming default argument? (I'm
> ambivalent about it)

Defaults to the c locale ::isspace.

> +    }
> +
> +    StringRef rtrim(StringRef Chars) const {
> +      return drop_back(Length - std::min(Length, find_last_not_of(Chars) +
> 1));
> +    }
> +
> +    StringRef trim(StringRef Chars) const {
> +      return ltrim(Chars).rtrim(Chars);
> +    }
> +
>
> +/// find_last_not_of - Find the last character in the string that is not in
> +/// \arg Chars, or npos if not found.
> +///
> +/// Note: O(size() + Chars.size())
> +StringRef::size_type StringRef::find_last_not_of(StringRef Chars,
> +                                                 size_t From) const {
> +  std::bitset<1 << CHAR_BIT> CharBits;
> +  for (size_type i = 0; i != Chars.size(); ++i)
> +    CharBits.set((unsigned char)Chars[i]);
> +
>
> cache the size in a loop variable here.
>
> +TEST(StringRefTest, Trim) {
> +  StringRef Str0("hello");
> +  StringRef Str1(" hello ");
> +  StringRef Str2("  hello  ");
> +
> +  EXPECT_EQ(StringRef("hello"), Str0.rtrim(" "));
> +  EXPECT_EQ(StringRef(" hello"), Str1.rtrim(" "));
> +  EXPECT_EQ(StringRef("  hello"), Str2.rtrim(" "));
> +  EXPECT_EQ(StringRef("hello"), Str0.ltrim(" "));
> +  EXPECT_EQ(StringRef("hello "), Str1.ltrim(" "));
> +  EXPECT_EQ(StringRef("hello  "), Str2.ltrim(" "));
> +  EXPECT_EQ(StringRef("hello"), Str0.trim(" "));
> +  EXPECT_EQ(StringRef("hello"), Str1.trim(" "));
> +  EXPECT_EQ(StringRef("hello"), Str2.trim(" "));
> +}
> +
>
> We can do a lot better testing than this. Cases that I would find
> interesting to pedantically check in the test:
>
> ""
> "  "
> "  \0  "
> "\0\0"
>
> StringRef("\0\0x\0\0").trim('\0')
>
> .trim("aaaaa")
>
>
> -Chandler

Committed with the changes you mentioned and above comments.

Thanks!

- Michael Spencer