[fixed_string] Gavin's review

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

[fixed_string] Gavin's review

Boost - Dev mailing list
This is my review of the proposed FixedString library.

- Do you think the library should be accepted as a Boost library?

Yes, I think it should be ACCEPTED into Boost.

Regarding the bikeshedding of the library/class name itself, I prefer
"fixed_capacity_string", as the most accurately descriptive name I can
either think of myself or that I have read posted by others.


I don't think we should be afraid of longer class names, even for
vocabulary types.  People can always alias them to something shorter to
save typing.


(Second choice would be "array_string", although that risks
misinterpreting as "string_array", which is something entirely
different.  "stack_string" is another possibility, though might be
misleading if contained within another object on the heap.)

- What is your evaluation of the design?

Overall, looks good.  I agree with the motivations for creating such a
type in the first place.

I would prefer if it didn't reinvent some existing storage concepts
already provided by boost::static_vector (and I don't entirely buy the
"reduce dependencies" arguments); however there are some other
justifications for this that I do consider valid (such as static_vector
not being constexpr).


As previously noted, I think implementing substr to return a string_view
is a mistake; this will cause lifetime problems if someone tries to
drop-in replace a std::string in existing code (which was one of the
motivating use cases).

I'm happy with this being implemented as a new method "subview".  (Maybe
std::string should do the same thing...)


However I still disagree with the choice to also provide "substr" but
with a fixed_string return type.  Its semantics (with regard to the
capacity of the returned string) are still sufficiently different from
std::string as to make its usage confusing and poor-performance-prone.

With the method omitted, someone attempting a drop-in-replacement will
get an error and will need to decide how best to handle it (usually by
using subview, but this may require other changes to extend lifetime,
which is why spelling subview as "substr" was bad).

And if they really want a copy of the substring, they can still do that
via subview simply by assigning it to a fixed_string variable -- this
would also require them to explicitly choose an appropriate capacity,
which I regard as a good thing.


For similar reasons to the above, my preference is also to entirely omit
operator+ (as in the current design).  The consumer can decide how to
rewrite things to use operator+= instead.

(While there are ways to implement operator+, as has been discussed, all
of these come with hefty caveats and I don't think a compromise is
sufficiently safe.)

- What is your evaluation of the implementation?

I did not have time to do more than glance at the implementation.

Although I did previously note (thanks to Deniz for calling attention to
it) that the unconditional #define BOOST_FIXED_STRING_USE_BOOST in
config.hpp is not the correct usage for configuration defines in Boost.

I also think that this could make better use of Boost.Config's existing
defines -- while a stated goal was to also produce a standalone library,
this could be done by using Boost.Config inside of Boost and using a
standalone mini-config header with equivalent defines only for the
standalone build.

I'm also dubious about the division of implementation between three
identically-named header files.  Unless this is required for
avoiding-ugly-docs reasons I think it would be preferable to merge these
into a single header -- and perhaps only a single header, where
Boost.Config is used.


Finally, though, I wonder if perhaps a variation of this type should be
implemented which has as storage only the array itself without a
separate size member.  This might better serve some serialization or I/O
scenarios (as a drop-in replacement for a raw char array), although with
the caveat that it would probably have to rely on strlen-equivalents and
thus could not contain embedded nulls (which is why it should be a
separate type, if implemented, as this is a larger departure from
std::string).
   As this would also typically have a smaller capacity than other use
cases it could do things like zero-init which might be too expensive for
the general-purpose fixed_string, but which can be very helpful for
serialization scenarios.

- What is your evaluation of the documentation?

The documentation is very short, but seems sufficient for the small
scope of the class.

- What is your evaluation of the potential usefulness of the library?

I think it will be extremely useful.

I do wonder if an immutable_string type would also be useful (especially
for constexpr), but that could be a separate library.

- Did you try to use the library?  With what compiler?  Did you have any
problems?

I did not try it.

- How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

A quick reading.  Probably a couple of hours in total spread over the
entire review period.

- Are you knowledgeable about the problem domain?

Yes, I frequently work on code where heap allocation is constrained or
disallowed.  And with strings.

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

Re: [fixed_string] Gavin's review

Boost - Dev mailing list
On Tue, Dec 3, 2019 at 10:52 PM Gavin Lambert via Boost
<[hidden email]> wrote:
> This is my review of the proposed FixedString library.

Thanks for taking the time to write a thoughtful review!

> I don't entirely buy the "reduce dependencies" arguments)
> ...
> I'm also dubious about the division of implementation between three
> identically-named header files.  Unless this is required for
> avoiding-ugly-docs reasons I think it would be preferable to merge these
> into a single header -- and perhaps only a single header, where
> Boost.Config is used.

I've adopted a new philosophy for libraries that they can be used on
their own when it is not overly burdensome to do so. FixedString is a
perfect candidate for this, as is my Boost.JSON. And that they can be
configured either for linking (default) or for header-only (opt-in).
This is not applicable to FixedString but it is how I am designing my
other libraries. The goal is to eliminate any barriers to using these
libraries.

The division of header files is to reduce the amount of header
material exposed to the documentation tooling and also for aesthetics.
However having a single header also has merit, it even further reduces
barriers to using the library. I agree that we should crunch this down
to one file, it is a good marketing tool.

Regards

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

Re: [fixed_string] Gavin's review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Gavin Lambert <[hidden email]> wrote:
> With the [substr] method omitted, someone attempting a drop-in-replacement
> will get an error and will need to decide how best to handle it

This is fine if someone is changing code that just uses
one string type, but it will break generic code that tries
to work with all string types, unless you modify that generic
code to require only the lowest common denominator.

Telling users that they can't have a substr() method because
they might use more stack space than they expected if their
fixed_strings are large is a bit patronising towards the users,
IMO.


Regards, Phil.





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