[fixed_string] Gavin's review
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.
On Tue, Dec 3, 2019 at 10:52 PM Gavin Lambert via Boost
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
Gavin Lambert
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.
participants (3)
-
Gavin Lambert
-
Phil Endecott
-
Vinnie Falco