[PATCH] D26319: Disallow StringRef assignment from temporary std::strings.

[Duncan Exon Smith via llvm-commits]

LGTM.

-- dpnes

> On Nov 4, 2016, at 17:18, Jordan Rose <jordan_rose at apple.com> wrote:
> 
> jordan_rose created this revision.
> jordan_rose added a reviewer: dexonsmith.
> jordan_rose added a subscriber: llvm-commits.
> jordan_rose set the repository for this revision to rL LLVM.
> jordan_rose added a dependency: D26317: Fix use-of-temporary with StringRef in code coverage.
> 
> Similar to https://reviews.llvm.org/D25446, this prevents accidentally referring to temporary storage that goes out of scope by the end of the statement:
> 
>  someStringRef = getStringByValue();
>  someStringRef = (Twine("-") + otherString).str();
> 
> Note that once again the constructor still has this problem:
> 
>  StringRef someStringRef = getStringByValue();
> 
> because once again we occasionally rely on this in calls:
> 
>  takesStringRef(getStringByValue());
>  takesStringRef(Twine("-") + otherString);
> 
> Still, it's a step, and it caught one real problem in Clang and one in Swift.
> 
> Depends on https://reviews.llvm.org/D26317
> 
> 
> Repository:
>  rL LLVM
> 
> https://reviews.llvm.org/D26319
> 
> Files:
>  include/llvm/ADT/StringRef.h
>  unittests/ADT/StringRefTest.cpp
> 
> 
> Index: unittests/ADT/StringRefTest.cpp
> ===================================================================
> --- unittests/ADT/StringRefTest.cpp
> +++ unittests/ADT/StringRefTest.cpp
> @@ -32,14 +32,50 @@
> 
> }
> 
> +// Check that we can't accidentally assign a temporary std::string to a
> +// StringRef. (Unfortunately we can't make use of the same thing with
> +// constructors.)
> +//
> +// Disable this check under MSVC; even MSVC 2015 isn't consistent between
> +// std::is_assignable and actually writing such an assignment.
> +#if !defined(_MSC_VER)
> +static_assert(
> +    !std::is_assignable<StringRef, std::string>::value,
> +    "Assigning from prvalue std::string");
> +static_assert(
> +    !std::is_assignable<StringRef, std::string &&>::value,
> +    "Assigning from xvalue std::string");
> +static_assert(
> +    std::is_assignable<StringRef, std::string &>::value,
> +    "Assigning from lvalue std::string");
> +static_assert(
> +    std::is_assignable<StringRef, const char *>::value,
> +    "Assigning from prvalue C string");
> +static_assert(
> +    std::is_assignable<StringRef, const char * &&>::value,
> +    "Assigning from xvalue C string");
> +static_assert(
> +    std::is_assignable<StringRef, const char * &>::value,
> +    "Assigning from lvalue C string");
> +#endif
> +
> +
> namespace {
> TEST(StringRefTest, Construction) {
>   EXPECT_EQ("", StringRef());
>   EXPECT_EQ("hello", StringRef("hello"));
>   EXPECT_EQ("hello", StringRef("hello world", 5));
>   EXPECT_EQ("hello", StringRef(std::string("hello")));
> }
> 
> +TEST(StringRefTest, EmptyInitializerList) {
> +  StringRef S = {};
> +  EXPECT_TRUE(S.empty());
> +
> +  S = {};
> +  EXPECT_TRUE(S.empty());
> +}
> +
> TEST(StringRefTest, Iteration) {
>   StringRef S("hello");
>   const char *p = "hello";
> Index: include/llvm/ADT/StringRef.h
> ===================================================================
> --- include/llvm/ADT/StringRef.h
> +++ include/llvm/ADT/StringRef.h
> @@ -215,6 +215,15 @@
>       return Data[Index];
>     }
> 
> +    /// Disallow accidental assignment from a temporary std::string.
> +    ///
> +    /// The declaration here is extra complicated so that `stringRef = {}`
> +    /// and `stringRef = "abc"` continue to select the move assignment operator.
> +    template <typename T>
> +    typename std::enable_if<std::is_same<T, std::string>::value,
> +                            StringRef>::type &
> +    operator=(T &&Str) = delete;
> +
>     /// @}
>     /// @name Type Conversions
>     /// @{
> 
> 
> <D26319.76957.patch>