[ Review ] [ fixed_string ] Accept

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

[ Review ] [ fixed_string ] Accept

Boost - Dev mailing list
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

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [ Review ] [ fixed_string ] Accept

Boost - Dev mailing list
Hi,

Thank you for the review! Here are a few comments:

On Wed, Dec 4, 2019 at 8:27 PM JeanHeyd Meneide via Boost <
[hidden email]> wrote:

> -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.
>

The name will be changed basic_foo_bar, and aliases will be added for
foo_bar/wfoo_bar etc. As for what foo_bar will actually be is TBD.

-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.
>

It will be implemented. I'm leaning towards just making the capacity be the
sum of the capacities of the operands (for fixed_string and array of
CharT). As for the other overloads (CharT*, string_view, etc) these
operations will either remain deleted, or just result in the type of the
fixed_string operand and throw if the capacity is exceeded.


> -0 - noexcept
>      It's 2019, everyone's getting on the noexcept train. Don't let it
> leave you behind!
>

I agree. For the most part, this has been taken care of on the live branch
(save for overloads that convert to string_view_type, but that will be done
very soon)

-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.
>

I agree, we should have constexpr, and that currently is my top priority.
However, there is the problem that pre C++20 constexpr requires
initialization of all members, which is undesirable.

Your suggestion to use a union with a dummy value unfortunately does not
work for C++11 and 14, since that would result in access to non-existent
array elements. In C++17, this is no longer the case, as when the left
operand of an assignment expression nominates a union member, it will begin
the lifetime of the member (in certain cases, there are a few restrictions
on the types that this can be done for). However, for C++11 and 14, there
currently exists no solution other than just zero-initializing the array.

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [ Review ] [ fixed_string ] Accept

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Also:

On Wed, Dec 4, 2019 at 8:27 PM JeanHeyd Meneide via Boost <
[hidden email]> wrote:

>  - 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.,
>

This has been implemented on the live branch

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|

Re: [ Review ] [ fixed_string ] Accept

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Krystian Stasiowski wrote:

> The name will be changed basic_foo_bar, and aliases will be added for
> foo_bar/wfoo_bar etc. As for what foo_bar will actually be is TBD.

The name as-is is actually better, but that might be another battle not
worth fighting. The standard convention is basic_foo template, foo/wfoo
non-templates. This is not the case here because everything is templated. We
can make it work because we have aliases now, but there's no real need to.


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost