On Thu, Sep 12, 2019 at 6:19 AM Mike via Boost
Not sure, if this counts as substantial...
Yes it does, pretty much anything that is not simply cosmetic (i.e. naming) is substantial :)
a) to make the string trivially copyable (simply copy the whole storage, not just the bytes actually containing the string). This might make things easier for the optimizer.
The purpose of fixed_capacity_string (see what I did there? LOL) is to facilitate allocation-free string mutation when the algorithm can reasonably impose an upper limit on the size of the resulting string. For example, Beast imposes an generous upper limit on some of the elements which make up the HTTP header. This allows the parser to use a stack based string to perform calculations and avoid allocations. To answer your question, no I have not considered the trivially copyable attribute when I wrote the class. We can certainly make it trivially copyable. Or not. I don't have a preference either way because my code does not copy such strings, and I do not know what the resulting performance characteristics will be for the use-cases where copying is needed. I can say that my strings are on the order of kilobytes, e.g. 1KB, 2KB, 4KB size strings. Copying all of a 4KB string when only a few hundred bytes are used may not be ideal, but absent experimentation it is hard to say if the tradeoff is worth it.
b) to conditionally enable support for std::string_view in c++17 (implicit conversion to and explicit construction from string_view)
Yes that is something that needs to be done. Beast kind of has support for this library-wide: https://github.com/boostorg/beast/blob/b7230f12f16fe7a9f7a1ece5be1f607c85524... I removed this option when I copied fixed_string into its own repository, because it does not feel right that EVERY library which wants to traffick in string_view has to invent its own configuration for deciding whether to use boost::string_view, std::string_view, or both. I have plans for more new repositories which need string views and it would be nice to have a Boost-wide solution - I'm very open to suggestions here. We should also think about the error_code related types from Boost.System, and the containers in Boost.SmartPtr for this sort of treatment. In Beast I did this: https://github.com/boostorg/beast/blob/b7230f12f16fe7a9f7a1ece5be1f607c85524... In theory I could change these aliases to use their std equivalents, but Asio gets in the way here since currently it can only use types from Boost.System.
Regarding the idea of making this less "templaty"
- I don't think is important enough to introduce a virtual function It makes things for the optimizer just so much harder.
- Given, that the full type has to be a template anyway, I'm very skeptical if any technique provides enough benefit in terms of compile times to warrant the additional overhead and maybe complexity that the necessary indirection would incur.
It can be done without the indirection:
class fixed_capacity_string_base
{
char* begin_;
std::size_t capacity_;
protected:
std::size_t size_;
fixed_capacity_string_base(
char* begin, std::size_t size, std::size_t capacity);
public:
//...
};
template
- The Traits template parameter might be a valid candidate for removal. I certainly never needed it, but my guess is that removing it makes only sense, if you also only support plain char (i.e. no wchar or char32_t) as a character type.
I can be perfectly happy supporting only `char`, but we could offer wchar and char32_t variants using explicit instantiation in the non-header-only configuration mode, and still get the benefits of separate compilation.
I like the to_fixed_string function btw.
Thanks! Composing integers as part of larger strings is a natural fit for fixed_capacity_string, e.g. "Content-Length: 2048\r\n" Regards