Page MenuHomePhabricator

Allow StringRef to be constructed with a null pointer
ClosedPublic

Authored by zturner on Sep 25 2016, 10:00 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 72438.Sep 25 2016, 10:00 PM
zturner retitled this revision from to Allow StringRef to be constructed with a null pointer.
zturner updated this object.
zturner added reviewers: lattner, mehdi_amini, pete.
zturner added a subscriber: llvm-commits.
lattner accepted this revision.Sep 26 2016, 9:18 AM
lattner edited edge metadata.

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

This revision is now accepted and ready to land.Sep 26 2016, 9:18 AM

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

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) { ... }

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.

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 edited edge metadata.Sep 26 2016, 11:06 AM

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.

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.

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.

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.

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.

This revision was automatically updated to reflect the committed changes.

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