Allow StringRef to be constructed with a null pointer

[zturner]

Summary

Making it official with a CL.

See http://lists.llvm.org/pipermail/llvm-dev/2016-September/105163.html for context and discussion.

Will wait a little while to commit this regardless of LGTM just in case someone has an objection.

[lattner]

LGTM, might as well use nullptr instead of 0 though.

[zturner]

The 0 is for the length, not the actual pointer. The pointer is just initialized with whatever you give to the constructor.

[clayborg]

Can we either leave in the:

StringRef(std::nullptr_t) = delete;

Or make the "const char *" constructor explicit? If we don't, we can end up getting default constructed StringRef objects when we don't expect them like in:

StringRef a("hello");
if (a == nullptr) { ... }
if (a == NULL) { ... }

[lattner]

The 0 is for the length, not the actual pointer.

Oh right, read it too quickly. I agree with Greg that keeping the deleted nullptr_t ctor is also sensible for this pass. It would be great to evaluate what happens when the const char* ctor is made explicit.

[zturner]

In D24904#552650, @clayborg wrote:

Can we either leave in the:

StringRef(std::nullptr_t) = delete;

Or make the "const char *" constructor explicit? If we don't, we can end up getting default constructed StringRef objects when we don't expect them like in:

StringRef a("hello");
if (a == nullptr) { ... }
if (a == NULL) { ... }

Leaving the deleted nullptr_t constructor seems reasonable. People shouldn't be checking against nullptr, they should be using .empty(), and this will allow the compiler to fail in that case. They can always explicitly invoke the default constructor if they really want it.

Marking the const char * constructor explicit means you won't be able to write foo("Test") anymore, which would be really unfortunate. There might be a way to hack around it with some overload / template magic, but I tried and couldn't get it to work.

[mehdi_amini]

Marking the const char * constructor explicit means you won't be able to write foo("Test") anymore, which would be really unfortunate. There might be a way to hack around it with some overload / template magic, but I tried and couldn't get it to work.

I have a patch that does that (adding the template ctor for string literal), and I'm updating the LLVM codebase locally before submitting.

[clayborg]

It we have the deleted nullptr constructor, it actually takes care of the "== NULL" case as well because it will complain it doesn't know which function should be used, so things still fails during compilation which is good.

[zturner]

In D24904#552657, @mehdi_amini wrote:

Marking the const char * constructor explicit means you won't be able to write foo("Test") anymore, which would be really unfortunate. There might be a way to hack around it with some overload / template magic, but I tried and couldn't get it to work.

I have a patch that does that (adding the template ctor for string literal), and I'm updating the LLVM codebase locally before submitting.

Mind if I take a quick look before you submit? I tried my darndest and couldn't get it to work, so idk if you're just better than me or if it doesn't work with MSVC. :) Either way I want to try it locally and see if it works.

[lattner]

You should be able to make string literals work if you use a template that takes an array of char with the size as a template parameter.

[zturner]

In D24904#552834, @lattner wrote:

You should be able to make string literals work if you use a template that takes an array of char with the size as a template parameter.

I thought so too, but in my tests given:

template<unsigned N>
void foo(const char (&S)[N]) { }

void foo(const char *S) { }

I could never get the compiler to invoke the first one, no matter what I passed in. It's possible I was missing some subtlety though, I'll just try it with Mehdi's forthcoming patch and see what I get.

[lattner]

This revision LGTM, making the "const char*" ctor explicit can & should be done as a second step, thanks!

llvm/trunk/include/llvm/ADT/StringRef.h

namespace llvm {
 
   public:
     /// @name Constructors
     /// @{
 
     /// Construct an empty string ref.
     /*implicit*/ StringRef() : Data(nullptr), Length(0) {}
 
+    /// Disable conversion from nullptr.  This prevents things like
+    /// if (S == nullptr)
     StringRef(std::nullptr_t) = delete;
 
     /// Construct a string ref from a cstring.
+    LLVM_ATTRIBUTE_ALWAYS_INLINE
     /*implicit*/ StringRef(const char *Str)
-      : Data(Str) {
-        assert(Str && "StringRef cannot be built from a NULL argument");
-        Length = ::strlen(Str); // invoking strlen(NULL) is undefined behavior
-      }
+        : Data(Str), Length(Str ? ::strlen(Str) : 0) {}
 
     /// Construct a string ref from a pointer and length.
     LLVM_ATTRIBUTE_ALWAYS_INLINE
     /*implicit*/ StringRef(const char *data, size_t length)
       : Data(data), Length(length) {
         assert((data || length == 0) &&
         "StringRef cannot be built from a NULL argument with non-null length");
       }