Make BasicStringRef compatible with std::experimental::string_view

[vitaut]

From reddit:

c_str generally assumes a NUL-terminated string of characters, however the constructor BasicStringRef(const Char *s, std::size_t size) makes no such assumption.

[CarterLi]

Unfortunately, Both BasicStringRef(const Char *s, std::size_t size) and BasicStringRef::c_str() are used currently.

There are 2 approaches to fix this issue:

1. Remove this constructor, thus all referenced string buffers are all null-terminated.
2. Deprecate c_str(). Use data() instead, which has no null-termination guaranty.
   Personally I prefer (2) which follows the behavior of upcoming std::experimental::string_view

There are other issues/improvements in BasicStringRef:

1. Lacks noexcept specifer
2. Compares pointer instead of value in operator ==
3. Adds more STL-type methods such as .begin(), .end() to enable C++11 foreach

Sorry for my bad English. Let me know if my words are hard to understand.

[vitaut]

I also prefer option (2) for the same reason. c_str() could be deprecated in the next minor release and possibly removed in version 2.0.

As for other improvements to BasicStringRef, they all look reasonable. Thanks for the suggestion.

P.S. I think your English is just fine.

[CarterLi]

Here is a WIP branch, which deprecates c_str():
https://github.com/CarterLi/cppformat/compare/StringBuf#diff-2fa4fa1309e5930bd844838042f3e2deR93

All unit tests passed though. The problem is: when I looked into the implementation detail, some code can be replaced directly, but code like https://github.com/cppformat/cppformat/blob/master/format.cc#L1066 and https://github.com/cppformat/cppformat/blob/master/format.h#L245 does assume that the referenced string buffer is NUL-terminated. Fixing them is not an easy work.

[vitaut]

The comparison and noexcept stuff looks great. However, if you plan to submit a PR for this, please don't introduce additional headers, as I try to minimize the number of required files to make inclusion in other projects as simple as possible. In the future, if the library grows too big, we could use some kind of a fusion process, like Google Test does, but until then I suggest keeping the main API in one header. Splitting the tests is fine though.

Also, I suggest keeping BasicStringRef minimalistic and don't try to replicate too much of string_view's functionality. Hopefully, string_view will become standard and we'll migrate to it at some point.

The cases like

  explicit FormatError(StringRef message)
  : std::runtime_error(message.c_str()) {}

should be pretty straigtforward to update:

  explicit FormatError(StringRef message)
  : std::runtime_error(message) {}

since StringRef provides implicit conversion to std::string. https://github.com/cppformat/cppformat/blob/master/format.cc#L1066 and some other cases do require more work.

Thanks for working on this!

[CarterLi]

New headers removed.
I won't work on it anymore. Please continue if you are interested.
https://github.com/CarterLi/cppformat/tree/StringBuf

[vitaut]

From #159:

If you are planning to support non-null terminated format strings, a constructor accepting a std::experimental::string_view (conditionally enabled with the __cpp_lib_experimental_string_view macro) with also help people using string_view use your library.

[vitaut]

On further investigation it appears that getting rid of c_str completely is impractical, because we need to pass null-terminated strings to C API functions, such as open. Moreover, using null-terminating strings makes handling the common case of C strings and std::string more efficient. string_view can still be supported in arguments.

[vitaut]

Fixed the main issue by adding BasicCStringRef, a class that represents a null-terminating string, and using it in cases that assume that the string is null-terminated such as POSIX functions and format string parser.

As a nice side-effect the per-call overhead is somewhat reduced bringing C++ Format even closer to printf. For example, the code fmt::print("{}", 42) now compiles to

0000000000000000 <main>:
   0:   48 83 ec 18             sub    $0x18,%rsp
   4:   be 02 00 00 00          mov    $0x2,%esi
   9:   bf 00 00 00 00          mov    $0x0,%edi
   e:   48 89 e2                mov    %rsp,%rdx
  11:   c7 04 24 2a 00 00 00    movl   $0x2a,(%rsp)
  18:   e8 00 00 00 00          callq  1d <main+0x1d>
  1d:   31 c0                   xor    %eax,%eax
  1f:   48 83 c4 18             add    $0x18,%rsp
  23:   c3                      retq

instead of

0000000000000000 <main>:
   0:   48 83 ec 18             sub    $0x18,%rsp
   4:   ba 02 00 00 00          mov    $0x2,%edx
   9:   bf 00 00 00 00          mov    $0x0,%edi
   e:   48 89 e1                mov    %rsp,%rcx
  11:   be 02 00 00 00          mov    $0x2,%esi
  16:   c7 04 24 2a 00 00 00    movl   $0x2a,(%rsp)
  1d:   e8 00 00 00 00          callq  22 <main+0x22>
  22:   31 c0                   xor    %eax,%eax
  24:   48 83 c4 18             add    $0x18,%rsp
  28:   c3                      retq

which is only 3 bytes more than std::printf("%d", 42):

0000000000000000 <main>:
   0:   48 83 ec 08             sub    $0x8,%rsp
   4:   ba 2a 00 00 00          mov    $0x2a,%edx
   9:   be 00 00 00 00          mov    $0x0,%esi
   e:   bf 01 00 00 00          mov    $0x1,%edi
  13:   31 c0                   xor    %eax,%eax
  15:   e8 00 00 00 00          callq  1a <main+0x1a>
  1a:   31 c0                   xor    %eax,%eax
  1c:   48 83 c4 08             add    $0x8,%rsp
  20:   c3                      retq

[vitaut]

Migrated the issue with comparison operators to #183.