Re: [boost] Fixed String review

Vinnie Falco wrote:

On Tue, Nov 26, 2019 at 8:31 AM Phil Endecott via Boost 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 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.