- Boost - lists.boost.org

Fixed String review
by Phil Endecott 02 Dec '19

02 Dec '19

Dear All, Here is my review of the proposed Fixed String library. In summary I think this would be a useful thing to have in Boost, but there are a few issues with the code as it currently stands. This is based on studying the code and docs and the previous mailing list discussions in September; I haven't actually tried to use the library. Firstly regarding the name, I would much prefer static_string for consistency with boost::container::static_vector and with P0843. Note that static_vector was the preferred name in an LEWG poll, according to P0843. It's true that "static" is an ambiguous word that does not really convey what is different about this string, but then neither does "fixed" in my opinion; "fixed capacity" would, but just "fixed" sounds more like "immutable" to me. On the subject of P0843, it it worth reading as it contains some useful links and rationale: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0843r2.html Also a more recent r3 update and reference implementation: https://github.com/gnzlbg/static_vector Here are the features that I think are missing, in no particular order; apologies if some of this does exist but I've missed it: - Much more of it should be constexpr. See P0980R1. That mentions that some of char_traits needs to be constexpr as a prerequisite, but even without that things like size() and empty() can be constexpr. There is also a question of whether all of the elements must be initialised (to 0) by the ctor to be constexpr, which could be costly for a large but sparse container, though it does mean that subsequent push_back()s don't need to store terminators; I don't understand all the issues involved here. (I note a comment in the docs suggesting the lack of constexpr may be temporary.) - In the same vein, more of it should be noexcept - why aren't empty() and size() ? - You can add a specialisation for size 0 that turns it into an empty struct. - You can add a string literal operator"". - You can add std::hash overloads. There are various possibilities for what should happen if the capacity is exceeded, including throwing an exception, asserting, and allowing undefined behaviour. I'm not sure what I would really prefer though I think I'd be more likely to assert() than to throw. See P0843r2 again which discusses this. Regarding your deviations from std::string: 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? 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. Regarding the docs, it would be great if the reference could have some highlighting (e.g. bright red?) for the few places where it differs from std::string. My own use for this sort of string has often been to store very large numbers of small strings when total memory usage has been a concern. (A good example would be an textual ID code of some sort, like a postal code or an airport code etc.) For this case, the 8-byte overhead for the size_t length field is a concern; I'd like a static_string<6> to need only 8 bytes total. I suggested changing this to the minimum size needed to store values 0 to N in the previous email discussion, but that hasn't been implemented. My initial thought was that static_string should be implemented on top of boost::container::static_vector<char>. My rationale for that was that static_vector is well-established and doesn't need to be re-implemented, and that a string_facade that adapts a vector<char>-like interface into a string-like interface would be a useful component in its own right. (I've tried to write something like this myself, but it gets tedious very quickly writing out all of those special string methods.) But then I discovered that actually boost::container::static_vector is not implemented as a simple size,data[] pair; apparently it contains pointers to its own storage. So it isn't even trivially- copyable. Now that I know that, I wonder if what we really need is a low-storage-overhead static_vector "done properly" (like the P0843 reference implementation), with a string_facade to make static_string from static_vector<char>. On balance, my view is that this proposal is promising but not quite ready yet. I suggest that the authors should be asked to come back in a few months with a revised version. My preferred design would be an improved static_vector and a string_facade that converts it into a static_string, but if they don't want to do that then simply tidying up the constexpr, noexcept and hash issues would be minimally sufficient. Regards, Phil.

9 23

Re: [boost] [Boost-users] [review][fixed_string] FixedString review starts today
by Peter Dimov 02 Dec '19

02 Dec '19

> This one at least does something: > > http://quick-bench.com/g_-BAIENMHQ1Pi1d136FBdp0idQ > > It still measures nothing though; the string isn't actually written. Finally, this one actually bothers to write the characters: http://quick-bench.com/-FUFkLtQeEa3brMf-y139G3Bbcg Quickbenches aren't very representative.

1 0

Re: [boost] [Boost-users] [review][fixed_string] FixedString review starts today
by Peter Dimov 02 Dec '19

02 Dec '19

> Wait, I forgot to increment the size! > > http://quick-bench.com/sI71NY7OpQ8ft27HpELko8q_4xM > > Not much improvement anyway. This one at least does something: http://quick-bench.com/g_-BAIENMHQ1Pi1d136FBdp0idQ It still measures nothing though; the string isn't actually written.

1 0

Re: [boost] [Boost-users] [review][fixed_string] FixedString review starts today
by Peter Dimov 02 Dec '19

02 Dec '19

> > Zach Laine wrote: > > Ok, I tried this myself on Quickbench: > > > > http://quick-bench.com/WZPNHFBKI-hFxThBYRZVjndtBho > > I tried to fix that to add the null terminator, but as a result both > push_backs were entirely optimized away. > > http://quick-bench.com/DtPR9Yov1TSzUFeYRx0ipVvUYBk Wait, I forgot to increment the size! http://quick-bench.com/sI71NY7OpQ8ft27HpELko8q_4xM Not much improvement anyway.

1 0

Boost 1.69 on Fedora/PPC64LE: tests results
by REIX, Tony 02 Dec '19

02 Dec '19

Hi, I'm testing Boost v1.69 on Fedora 31 / PPC64LE , as explained at: https://www.boost.org/development/running_regression_tests.html And I see many errors: 1 Failed test out of about 11 Run tests: # cd status # ../b2 ... Passed 101532 tests Failed 10368 tests 90.7% of tests completed sucsessefully Passed 1188 tests Failed 15 tests 98.8% of tests completed sucsessefully I do not know what I made wrong. So, since I do not see any Fedora/PPC64LE machine in the BuildFarm: https://www.boost.org/development/tests/master/developer/summary.html I'd like to know where I could find the summary of the official tests of Boost 1.69 on Fedora/PPC64LE. Thanks/Regards, Tony Reix

1 0

Matrix Multiplication
by Siddhartha Sen 01 Dec '19

01 Dec '19

Having gone through quite a few libraries, the ones that intrigued me the most are Boost.Algorithms and Boost.GIL. I had an idea for Boost.GIl in fact. Basically what I wish to propose(please do correct me if it already exist) is a library that will have the functionality to convert a image to a negative one (I found the conversion of colour image to black and white functionality, but not this). Also, the process can be massively sped up by the incorporation of parallel programming paradigms like OpenMP. Do let me know what you guys think about this.

1 0

[fixed_string] Why "fixed" rather than "static"?
by Andrzej Krzemienski 29 Nov '19

29 Nov '19

Hi Everyone, First, I would like to thank Vinnie and Krystian for writing and sharing the library. While this is not a review, I would like to share a thought about the choice of name. Given that we have static_vector, which seems to be doing a similar thing, why was this library called fixed_string rather than static_string? I note that name fixed_string has some potential to mislead the users as it might imply that it is the size that is fixed (like "a string that always stores 2-letter country codes"). I am not saying that the name choice is wrong. I would just want to see the rationale. Regards, &rzej;

5 8

Re: [boost] [Boost-users] [review][fixed_string] FixedString review starts today
by Peter Dimov 29 Nov '19

29 Nov '19

> and yes, you can write code that will be correct if op+= doesn't check. Maybe you can, but I apparently can't. This is what I just wrote: void my_append( fixed_string<512> & s, std::string_view s1, std::string_view s2 ) { if( s.size() + s1.size() + s2.size() > s.max_size() ) throw std::length_error( "" ); s += s1; s += s2; } Is this correct? (Spoiler: no.)

2 2

Re: [boost] [Boost-users] [review][fixed_string] FixedString review starts today
by Peter Dimov 29 Nov '19

29 Nov '19

> https://godbolt.org/z/Z37Pmx > https://godbolt.org/z/CXHRKQ I forgot to maintain a 0 terminator in this sketch; it's even worse with it. .L5: add rax, 1 movzx ecx, BYTE PTR [rdx] cmp rax, 512 ja .L11 add rdx, 1 mov BYTE PTR [rdi+7+rax], cl mov BYTE PTR [rdi+8+rax], 0 mov QWORD PTR [rdi], rax cmp rsi, rdx jne .L5 https://godbolt.org/z/nhYZQY .L3: movzx ecx, BYTE PTR [rdx+rax] mov BYTE PTR [rdi+8+rax], cl add rax, 1 mov BYTE PTR [rdi+8+rax], 0 mov QWORD PTR [rdi], rax cmp r8, rax jne .L3 https://godbolt.org/z/pGJRWs

1 0

Re: [boost] [review][fixed_string] FixedString review starts Nov 25
by Peter Dimov 28 Nov '19

28 Nov '19

> Also, a synopsis section in the documentation might have been nice, to > allow one to see all the declarations in one place. (Admittedly, the main > header does serve that purpose somewhat.) I join the others in noting the pervasive lack of constexpr and noexcept. Even the default constructor isn't either. Can't think of a reason why it would throw. (The lack of constexpr is much easier to explain.) Also, Traits template parameters were wrong in C++98, and they remain wrong today, even though the committee has propagated that mistake into string_view.

2 1