- Boost - lists.preview.boost.org

[review][fixed_string] Andrzej's feedback
by Andrzej Krzemienski 05 Dec '19

05 Dec '19

Hi Everyone, This can hardly be called a review, as I didn't look thoroughly at the implementation. (I only checked what happens on the resize() over the capacity(), as this was not clear from the docs.) I think that ultimately the library would be a useful addition to Boost, however I would not like to rush it before its design is fully baked, and my impression is that it isn't yet. Therefore my recommendation would be to REJECT the library at this point, and encourage the authors to resubmit it for the second review once the design goals and design decisions have been clearly settled and communicated. My vision of a review is that we: 1. First evaluate the design goals, whether they are worth pursuing 2. Then we evaluate the design *against* those goals. 3. Then we evaluate the documentation and implementation *against* the design and goals. I cannot even go to #3, as to me #1 is missing some parts. There are different possible goals: 1. Treat this new string as many other STL sequence containers with variable size. In particular, users use it as if its max_size() were infinite, and are prepared that at some point an exception might be thrown. 2. Treat it as a "drop-in replacement" for std::string with no-heap-alloc guarantee. 3. Make something suitable for embedded systems where the user provides the guarantee: "I will never make the size() of this string go above this value. If I did, it would be plain clear that the program is not doing what it was supposed to." These goal 3 is in clear conflict with either 1 or 2. This becomes clear when we consider what should be the response to doing a resize() over capacity(): a precondition violation or a normal course of action signaled by an exception. The authors should make a choice which of these goals is not supported and communicate it clearly from the outset in the docs. I think this decision has already been made: the library was extracted from Boost.Beast and there the purpose must have been clear. It is just that it is not communicated in fixed_string library. Any decision is good. In C++ performance trade-offs are part of the contract (interface), so there is room in Boost for many strings: small strings, short codes, fixed size strings, fixed capacity strings. But for each library it should be clear what trade-offs were chosen and why. We have a situation that there exists static_vactor in Boost, which defines the resize() over capacity() to be a precondition violation. And fixed_string needs to provide some response to this fact: why did you choose a different design? I consider the choice of name to be part of the design: you cannot assign the name before you have decided on the goals. "static_string" might be a good name if you chose the same trade-offs as static_vector. But if you make different trade-offs, it might be better to use a different name. Making a resize() over capacity() a precondition violation is a *feature* useful for bug detection and I do not consider it a valid argument that "library will throw exceptions and if you never resize() over capacity() you will never see exceptions or std::abort()". If this is a precondition, then I expect of a library to put some BOOST_ASSERT() or _builtin_unreachable() in those paths to enable better bug detection. I find the goal "a drop-in replacement for std::string" invalid. You surely mean only a drop in replacement for some subset of usages. I guess this subset comes from the usage in Beast, and I would expect that it is clearly outlined what this subset is. One of the operations on strings that I use most often is operator+, so when I hear that this operation is not implemented in a class that claims to be a drop-in replacement for string, I see a contradiction. There might be valid reasons not to provide operator+, but the documentation cannot state that this is a drop-in replacement for std::string. I think that only after the above issues have been sorted out can we have a review of the implementation and the documentation. One remark about documentation is that I was not able to find what `resize()` over `capacity()` does. This is the most important information that I was after. anything else is more less intuitively clear from the analogy to std::string. I hope that this feedback doesn't sound discouraging. I appreciate the work Krystian and Vinnie have put into making this submission, and I would like to see this library ultimately included in Boost. Regards, &rzej;

7 27

iostreams gzip_compressor
by Kemin Zhou 05 Dec '19

05 Dec '19

I have been using the compressor and decompressor for a while now. One of the issues is the performance not sufficient for large data operations. The underlying zlib library is single-threaded, and there is a parallel version implementation at application level https://zlib.net/pigz/ pigz. When working with large file compression and decompression it usually take 8 hours to do the job. If I can use multi-threaded version then it usually can be done within one hour. There is a strong need to use multi-threading to speed up this process for me. I would appreciate that someone can implement this feature. If not I can work on this project. Not sure what's the best way to go about this. -- Kemin Zhou 858 366 8260

1 0

[fixed_string] Copying/comparing entire strucutre
by Phil Endecott 05 Dec '19

05 Dec '19

Dear All, Considering small-capacity fixed_strings like the example that I gave previously: struct NameAndAddress { static_string<14> firstname; static_string<14> surname; static_string<30> address_line_1; static_string<22> city; static_string<10> postcode; }; I have been looking at how best to copy and compare them. For small-capacity strings, and also for large-capacity strings when their size approaches the capacity, it will be quicker to unconditionally copy/compare everything, including the unused bytes beyond the end, rather than looping over just the current size. I've tried a very crude benchmark based on this: struct string { uint8_t size; std::array<char,N> data; }; extern void copy_all(string& dest, const string& src) { dest = src; } extern void copy_size(string& dest, const string& src) { dest.size = src.size; std::copy(src.data.begin(), src.data.begin()+src.size, dest.data.begin()); } Results will depend on the architecture but I found that for N < 64 it was probably always best to copy_all(). For N==255, time for copy_all() and copy_size() is about equal when the container is half full. Things are more complicated for operator== because of the unused bytes. It would be necessary to initialise everything to 0 and maintain that during e.g. erase() and assign(). I haven't tried to benchmark that but I suspect the trade-off would be similar. operator< is a mess due to endianness. I don't know whether any compilers or memcmp() implementations are smart enough to use byte-swap instructions to process data 4 or 8 bytes at a time. I don't think I'm suggesting that this should necessarily be implemented in fixed_string, at least not without lots more investigation, but it does illustrate more of the trade-offs that exist between different applications. Regards, Phil.

2 1

[ Review ] [ fixed_string ] Accept
by JeanHeyd Meneide 05 Dec '19

05 Dec '19

Dear Boost, This is a review of boost.fixed_string. It is my first review, so take it with a grain silo's worth of salt. ------------- Summary 📝 -------------- Accept fixed_string. It's about ---------------- What I Did ⚙️ ---------------- - I ran its tests, VS 2019 i686 and x86-64 | VS 2017 i686 and x86-64 | MinGW 9.2.0 i686 and x86-64 - I put it inside itsy.bitsy (https://github.com/ThePhD/itsy_bitsy) and used it as an underlying container in its tests. - I put it inside my private text repository as an underlying container to hold code units for several different encoding schemes and it work (https://github.com/ThePhD/text-dev | far-behind implementation at https://github.com/ThePhD/text) - I generally messed around with it in a basic main.cpp file and read the docstrings and docs while comparing it to cppreference. -------------- Strengths 💪 -------------- +1 - Follows std::string's container design nicely, making it easy to program around and know what to expect from the outset. (Side note: ignore whoever said to remove the initializer list ones: I used all of them. It's necessary to be considered a SequenceContainer (https://en.cppreference.com/w/cpp/named_req/SequenceContainer).) +1 - Works with all character types. char, wchar_t, char16_t, char32_t, and -- in C++20 mode -- char8_t for the compilers that have it. +1 - insert, erase, clear, push_back, push_front meet specification I flexed it by inserting it as part of itsy.bitsy's tests and using it as one of its sequence containers. +1 - docs are straightforward. +1 - iterators work just fine. I tested it using itsy.bitsy as an underlying container for a reference_wrap'd bit_view as well as a stress testing it under my phd::text implementation. It worked fine as an underlying container for UTF8, UTF16. UTF32, and UTF-7-EBCDIC storage. +5000 - Standalone. I wish more Boost libraries were packed this way. I can make it not depend on Boost as a whole; great for dropping into random codebases and using as a replacement. Also compiles faster for those of us who have C++17+ capable compilers. ------------------- Weaknesses 🤒 ------------------- -1 - Naming. fixed_string<500> works, wfixed_string<500> doesn't, basic_fixed_string<char, 5000> doesn't either. It uses a different naming scheme from the standard, and led me around in circles on first use until I forced myself to read the template head of the class declaration. But it's Boost, so we can afford to be different and stuff. Just don't do static_vector. :D -0 - operator+ I wanted it. It wasn't there. I can see why, since you would have to pessimistically expand the size to the max between the two. Do enough additions and you can explode the final result's stack size several, so this isn't really a loss. Besides,-0 is just 0 on any reasonable 2's complement machine. -0 - noexcept It's 2019, everyone's getting on the noexcept train. Don't let it leave you behind! -2500 - Constexpr is lacking. I could not compile any of itsy.bitsy's constexpr, compile-time, or static_assert tests. I could not compile any of the static_assert tests from latest private text development. This prevents a wide range of applications, including my failure to substitute boost.fixed_string for Hana's Fixed String implementation in CTRE and failure to use fixed_string as a compile-time buffer for encoded and decoded text. -------------- Misc. -------------- - I see you added subview, very nice. 👏 - substr should continue to return a fixed_string. - The empty optimization and size optimization is important. Don't listen to anyone who tells you fixed_string<0> should report false for std::is_empty_v<fixed_string<0>>. I spend LITERALLY MILLIONS OF YEARS (definitely literally) programming special __ebco<T, tag> or slapping [[no_unique_address]] on things to cut down on memory sizes. Please don't cause a cascade of bloated size with MSVC's current crappy base class """optimization""" rules (the new, better ones are still opt-in because yay, ABI breaks). The closer things are to being small enough or trivial enough that they get register-passed, the better. - Speaking of the size optimization, MSVC's standard string also employs a trick where they shove the size in the last CharT. May be of use to you here, but probably not worth the efforts. -------------- Unrelated -------------- The following is boring things not related to the review but stated here for extra flavor that can be completely ignored: - Boy that CMake file hurts my feelings. I fixed it up so I could use it with add_subdirectory. It promised me an easy integration and smacked me upside the head instead. But at least I could get the tests going soon enough. Maybe don't commit it, it gives the impression it's for public consumption (it's not). - Tests are slightly inscrutable, better names would be nice. - For constexpr: use a union of { char derp; CharT value; } and value-initialize the char. Provide iterators that walk over the union and on deref directly handle you the CharT& lvalue reference. .data() is unimplementable as constexpr like this but who cares: you have a constexpr begin() and that's what people should be using. .data() is for type-punning shenanigans. - You should turn up the warnings on MSVC: there's a few places where there's an implicit narrowing thanks to going from size_type (std::size_t) to smallest_width_t and MSVC gives a snippy warning. Overall, yay! Try to get the constexpr stuff in quick; that's what's going to make this library interesting to use for C++20 and beyond, and have Boost libraries start being things to look forward to, rather than representing the Old, Boring Establishment that I reach for when I hear the words "Sea Plus Plus Oh Three". Great Job, JeanHeyd Meneide

3 3

[release][proto]: MSVC 19.24.28314 (VS 2019 16.4.0) )won't compile proto's generate.hpp
by Löw, Tobias (STEAG Energy Services GmbH) 05 Dec '19

05 Dec '19

Hi, the latest MSVC version (shipped with VS 2019 16.4.0) doesn't compile boost/proto/generate.hpp since there is an old MSVC-fix, which is no longer needed, and now even produces errors. The "develop" branch of proto contains a fix for that since 2015 (!) but it never made it to the master branch. (https://github.com/boostorg/proto/issues/20) Could some please merge it over to make it into release 1.72 - would be really great! Tobias

1 0

[review][fixed_string] FixedString review starts Nov 25
by Joaquin M López Muñoz 05 Dec '19

05 Dec '19

The Boost formal review of Krystian Stasiowski and Vinnie Falco's FixedString library will take place November 25 - December 4, 2019. This is an early call for you to consider participating in the review. Feedback and constructive criciticism from experts and not-so-experts alike is very much welcome. FixedString being a relatively small and straightforward library, you will find it very easy to familiarize yourself with it and contribute back to the community. FixedString provides a dynamically resizable string of characters with compile-time fixed capacity and contiguous embedded storage in which the characters are placed within the string object itself. Its API closely resembles that of std::string. GitHub repository: https://github.com/18/fixed_string Online docs: https://18.github.io/doc/fixed_string If you feel like reviewing FixedString you can start now prepping --though I'd ask you to refrain from submitting actual reviews till Nov 25. Read the docs, download/fork the code, read/run it, maybe try integrating it into some project of yours, ask questions to the authors, preferrably on Boost or Boost Users mailing lists. The FixedString initiative arose from a very lively discussion within the community: https://lists.boost.org/Archives/boost/2019/09/246943.php , so I sincerely hope this review will be a fruitful, crowded one. Don't let me down! Best regards, Joaquín M López Muñoz Review manager for candidate Boost.FixedString

5 5

Generic Image Library
by Siddhartha Sen 05 Dec '19

05 Dec '19

Being new to this, I have tried looking up the functionalities of GIL. I was thinking of implementing the addition of negative image conversion as well to GIL, incorporating the usage of parallel programming using OpenMP library. Is there any demand for that? I feel that parallel programming will massively improve the computational efficiency of GIL.

2 1

[review][fixed_string] review period extended to Dec 8
by Joaquin M López Muñoz 05 Dec '19

05 Dec '19

After conversation with the review wizards, the review period for candidate Boost.FixedString is extended till Sunday, Dec 8. Hopefully this will allow for some more reviews to come in and for the ongoing discussions to stabilize. So, if you're considering to submit your review, now is the time to reserve some time to do so. Thanks to everybody who has contributed so far, it is a very productive review we're having. Joaquín M López Muñoz Review manager for candidate Boost.FixedString

1 0

Re: [boost] [fixed_string][review] Review of fixed_string
by Peter Dimov 05 Dec '19

05 Dec '19

> - everything should ideally be constexpr. The problem here is that > constexpr requires initializing all elements, and this will heavily > penalize runtime uses of f.ex. fixed_string<512>. On compilers with > __builtin_is_constant_evaluated (gcc 9, clang 9) we should use that; > otherwise, it might be worth it to create a specialization for N < some > suitable upper limit. Wait, I have an idea. template<std::size_t N> struct fixed_string { private: std::size_t size_ = 0; union { char first_ = 0; char data_[ N+1 ]; }; public: fixed_string() = default; }; Now default initialization sets just the first char to zero, as we want, and value initialization initializes all data_ to zero and is constexpr. auto f1() { fixed_string<24> s; return s; } auto f2() { constexpr fixed_string<24> s{}; return s; } https://godbolt.org/z/uhf4S4 I don't know if that's defined behavior or not, but if it isn't, it better be.

2 1

[review][fixed_string]
by Peter Koch Larsen 04 Dec '19

04 Dec '19

Dear boosters Here is my review of fixed_string > - What is your evaluation of the design? I have just completed a similar but much less ambitious design - about 700 LOC in total. The purpose is to replace char string[N] representing string in an ageing code base that is C++98 currently and partly embedded. My design does support a partial compatibility with std::string but that was not the primary goal. The interface compatibility of std::string is a nice feature, but I do not see a reason to attempt to make it a drop-in replacement. The non-compatibility with char[N] is missed by me and probably also some other in the embedded domain. It would be nice if fixed_string<N> had size = N + 1 for char types. > - What is your evaluation of the implementation? Nice and clean. As others have noted, constexpr and noexcept support would be nice. I would also prefer if I could have an option that does not require usage of exceptions, perhaps as a configuration option. Specifically, I would like to be able to control error handling (terminate instead of throw, ability to react to undefined behaviour). Also, I would like to have more forgiving functions that could return the status of failing operations. > - What is your evaluation of the documentation? Nice and clean. > - What is your evaluation of the potential usefulness of the library? If the interface could be extended, It could potentially be useful to many - and to me - if I manage to convince my company to ugrade to C++17. So to summarize: 1) compatibility with char[N]: this requires that the size member is removed. In my source, I still have an O(1) size implemented by having data[N] give the free space in the string. If data[N] == 255, this indicates that the real free size is stored in the elements data[N - 1] and below. There are many ways to encode that length portably. 2) append and assign could have companion functions, that behave in spirit like strncpy/strncat. I have named these function try_append and try_assign. 3) Errorhandling strategy available. > - Did you try to use the library? With what compiler? Did you have > any problems? No, just reading code. > - How much effort did you put into your evaluation? A glance? A quick > reading? In-depth study? About three hours reading and writing this comment. > - Are you knowledgeable about the problem domain? Yes. > > And finally, every review should answer this question: > - Do you think the library should be accepted as a Boost library? The constexpr and noexcept parts should be fixed and compatibility with char[] would be nice. But yes, it should be accepted.

1 0