kimgr added a comment.

This is probably not the right place for this discussion, but I thought I'd 
offer one more note.


================
Comment at: include/string_view:216
@@ +215,3 @@
+       basic_string_view(const _CharT* __s)
+               : __data(__s), __size(_Traits::length(__s)) {}
+
----------------
mclow.lists wrote:
> kimgr wrote:
> > mclow.lists wrote:
> > > mclow.lists wrote:
> > > > kimgr wrote:
> > > > > mclow.lists wrote:
> > > > > > kimgr wrote:
> > > > > > > I'm working from the paper at 
> > > > > > > https://isocpp.org/files/papers/N3762.html, and I find it a 
> > > > > > > little sketchy on the policy for nullptrs.
> > > > > > > 
> > > > > > > Since the ctor above accepts nullptr as long as the length is 
> > > > > > > zero, would it make sense to do that here too? That is, only call 
> > > > > > > _Traits::length for non-nullptr __s args?
> > > > > > Reading from N4600: Requires: `[str, str + traits::length(str))` is 
> > > > > > a valid range.
> > > > > > 
> > > > > > So, no - passing `nullptr` here is undefined.
> > > > > > 
> > > > > OK, that feels more principled to me, anyway.
> > > > > 
> > > > > But the `(const char*, size_t)` constructor has the same requirement 
> > > > > and it *does* accept `nullptr`, provided the length is zero. I saw 
> > > > > you had to get rid of the assertion, but I feel like the constructor 
> > > > > should probably not silently accept `nullptr` for zero-sized strings. 
> > > > > Or do you know if that's intentional? Many StringRef/StringPiece 
> > > > > implementations seem to have the same behavior.
> > > > It is absolutely intentional. `[nullptr, nullptr+0)` is a perfectly 
> > > > fine half-open range. It contains no characters.  
> > > > 
> > > > However, the ctor that takes just a pointer has to calculate the length 
> > > > .. by dereferencing the pointer.
> > > > 
> > > > I had to get rid of the assertion because one of the bots (gcc 4.9) has 
> > > > a bug about constexpr ctors in c++11 mode.  Even though the assertion 
> > > > was `#ifdef`ed on C++ > 11, the compiler complained about it.  I'll be 
> > > > putting the assertion back as soon as I can figure out how to keep gcc 
> > > > from complaining.
> > > This was discussed (at length) in LEWG during the design of `string_view`.
> > Ah, got it, thanks! It opens up for cases where `data()` for an empty 
> > `string_view` can sometimes return `""` and sometimes `nullptr`, but I 
> > guess that problem extends beyond `string_view`'s responsibilities.
> > 
> > Thanks for the thorough explanation.
> I think you're laboring under a misapprehension here.
> 
> An empty `string_view` points to *no characters*, not an empty 
> null-terminated string.
> 
> Treating the pointer that you get back from `string_view::data` as a 
> null-terminated string will lead to all sorts of problems.  This is 
> explicitly called out in [string.view.access]/19:
> 
> > Note: Unlike basic_string::data() and string literals, data() may return a 
> > pointer to a buffer that is not null-terminated. Therefore it is typically 
> > a mistake to pass data() to a routine that takes just a const charT* and 
> > expects a null-terminated string.
> 
> 
Thanks, I think I'm free of that particular misapprehension :-)

The case I had in mind was:

    void f(const string_view& s) {
      char buf[50] = {0};
      assert(s.size() < 50);
      memcpy(buf, s.data(), s.size());
    }

There's no assumption of null-termination here, but as I understand it, 
`memcpy` has undefined behavior if source is `nullptr`, irrespective of whether 
`num` is zero or not.

So these would be valid:

    f(string_view("", 0));
    f(string_view(""));

but this wouldn't:

    f(string_view(nullptr, 0));

which I find slightly asymmetric.

But again, maybe this is not `string_view`'s fault, but `memcpy`'s, in which 
case `nullptr` kludges could be moved to wherever `memcpy` is used instead of 
wherever `string_view::string_view` is used.


https://reviews.llvm.org/D21459



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to