Re: Fixed String review

From: Phil Endecott (spam_from_boost_dev_at_[hidden])
Date: 2019-11-27 16:36:34

Vinnie Falco wrote:
> On Tue, Nov 26, 2019 at 8:31 AM Phil Endecott via Boost
> <boost_at_[hidden]> wrote:

>> - Much more of it should be constexpr.
> Taking the opposite position, why should it be constexpr?

I think best practice is to declare everything constexpr unless 
it can't be.  It's a bit like const was 20 years ago - does anyone 
else remember dealing with old code that didn't declare anything 
const?

>> operator+ has been omitted because you consider it's not possible
>> to determine a reasonable return type; isn't it just size N+M ?
>> Do you reject that just because it could end up rather large after
>> a sequence of additions?  I'd agree if it were N*M but not really for
>> N+M.  Have I missed something?
>
> Yes. It is too easy for someone to change existing code that does
> a+b+c+d where the operands are strings. Users could the get surprising
> behavior of tens or hundreds of kilobytes of stack consumption. There
> are other ways to implement operator+ but we prefer to the omit
> interfaces that can have surprising behavior, or that have non-obvious
> results (such as truncating).

Your application seems to be for largish buffers; mine is for 
smallish strings.  For the smallest strings a static_string 
can be smaller than a std::string, and for slightly larger 
strings the space overhead if the actual string is shorter 
than the capacity may be an acceptable trade-off for the 
improved performance.  For example:

struct NameAndAddress {
  static_string<14> firstname;
  static_string<14> surname;
  static_string<30> address_line_1;
  static_string<22> city;
  static_string<10> postcode;
};

(Yes, yes, you probably wouldn't want to make those fields fixed-size 
in real code, but it's an example we can all understand.)  That struct 
is trivially-copyable.  Copying a version that used std::string would 
involve multiple memory allocations and could be an order of magnitude 
slower.  Now:

std::string a = s.firstname + " " + s.surname + ",\n"
              + s.address_line_1 + ",\n"
              + s.city + ",\n"
              + s.postal_code;

That will create a temporary static_string<97> and copy from that 
into the std::string.  But it needs a series of temporaries and probably 
uses a few hundred bytes of stack.  Yes this isn't as good as we could 
get from a concat(...) function (or expression templates) that computed 
the size of the required buffer up-front and copied all of the arguments 
once, but it's still going to be an improvement over using std::string's 
operator+.

>> - You can add std::hash overloads.

> I'm not seeing the value in these features. What's the use case? No
> one will be using static_string as the key for a container, since it
> wastes space.

using customer_code = static_string<11>;
std::unordered_map<customer_code,NameAndAddress> customers;

Vinnie, are you saying that this (i.e. small strings that are trivially-
copyable) is not a supported use for the library?  If you are, please say 
so and we can just reject it!

>> You've chosen to return a string_view from substr().  I think that's
>> a bit dangerous; code that does "auto a = s.substr(...)" could end
>> up with a dangling view when s is a fixed string; this makes it
>> more difficult than it should be to migrate from one string type
>> to another, or to write generic code.
>
> Again we have to refer to the purpose of the container.

Well again I'll contend that your purpose for the container is not the 
only one.

> People are
> using this for performance, the very last thing they want from
> substr() is to receive a copy. Users can opt-in to making a copy if
> they want, by constructing a new static_string from the string view
> returned by substr. If we go with your suggestion, no one can opt out
> of copies.

If you want to get a string_view of a substring of a std::string, I think 
you can write something like

std::string s = .....;
std::string_view sv(s);  // view of all of s
std::string_view sub_sv = sv.substr(pos,count);  // view of a subview of s

Or in one line:

auto sub_sv = std::string_view(s).substr(pos,count);

Maybe std::string should have a method to do that more succinctly?

Anyway, someone wanting to get a string_view of a substring of a 
fixed_string can do exactly the same thing.

>> My initial thought was that static_string should be implemented
>> on top of boost::container::static_vector<char>. My rationale...
>
> No normal user is going at static_string and think "I would use this
> if it required Boost.Container."

Vinnie, I have literally just said that I'd prefer this to be an 
improved static_vector and then an adaptor that adds all the string 
methods on top of static_vector<char>.  I think that would be better 
because it gives us three useful things "for the price of one", i.e. 
a better static_vector, a static_string, and a "stringification" 
adaptor that can be used independently.  So you're calling me 
"not normal" for suggesting that.  I think that's rude.  You've 
also been rude to Gonzalo Brito Gadeschi and the whole of WG21 in 
parts of your message that I've not quoted.  Please don't do that.

Unfortunately I don't have the time to respond to everything else. 

Regards, Phil.