[FixedString] Deniz' review

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

[FixedString] Deniz' review

Boost - Dev mailing list
This is my review of Krystian Stasiowski and Vinnie Falco's FixedString
library.

The important question and its answer upfront:

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

I think it should be ACCEPTED into Boost.

It is a very small (almost trivial) library and in the beginning I was
not sure if it really should go into Boost. However, for me it looks
like a useful building-block, so I vote ACCEPT.
And if I remember correctly Vinnie said he is already having some other
libraries using it which he wanted to propose for Boost later, so there
seem to be some real use-cases for it already.
Besides, Boost already has some other smaller building-block libraries,
so FixedString should be in good company.


On a side-note: It would be helpful for the Boost library documentation
overview (e.g. https://www.boost.org/doc/libs/1_71_0/) to allow more
views/sort-orders for listing available Boost libraries than the
views/sort-orders which are already there. (Having "building-blocks" and
especially "newer C++ standard features for C++03" or similar "standard
compatibility" categories for the Boost libraries comes to mind.)
OK, now back to the review.


- What is your evaluation of the design?

The overall design looks straight forward.
`boost::fixed_string`'s API resembles that of `std::string` and only the
internal representation differs largely. This looks like the correct
design for the aimed purpose of this library (to provide a dynamically
resizable string with compile-time fixed capacity.)


- What is your evaluation of the implementation?

The implementation looks solid and also straight forward. However, I
have some remarks to make which you can read further down in this email.


- What is your evaluation of the documentation?

The documentation is quite small, except for the reference section.
This, however, should be fine considering the very simple and small
focus which FixedString has.

Regarding the "Iterators" section, it was not clear to me on first read
for which string type the iterators are invalidated when moving/swapping.
I think, `boost::fixed_string` is meant and not `std::string`, but that
was not obvious from first read and the wording should probably be
improved there to be unambiguous. (Maybe make this clear by speaking of
"FixedString" or "fixed_string" instead of just "string".)


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

For the use-cases mentioned in the "Motivation" section of the
documentation it seems to be useful. All other use-cases I could think
of are specializations of the mentioned use-cases.


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

No, I have not tried to use the library.
However, I am curious to know if Boost.String_Algorithms can operate on
`boost::fixed_string`, too. If not, it would be nice for the code of
FixedString and/or Boost.String_Algorithms to be modified in such a way
that they can inter-operate with each other.


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

I read the documentation, but the reference section I merely visually
scanned through. (I assume, there are no mistakes in it, as they mainly
resemble the API of `std::string`.)
I looked at the implementation but did not look at all function
implementations in all depths. I, however, looked at the CMakeLists.txt
file, too, and have some remarks for it (see below).

Overall, I probably spent about 1.5 hours for the review, not including
the writing of this email.


- Are you knowledgeable about the problem domain?

Only as far as it gets if you are a general user of `std::string`.



I have the following additional remarks:


* `boost::to_fixed_string` / `boost::detail::max_digits`:

- Is the returned `boost::fixed_string` type of `boost::to_fixed_string`
large enough to also hold the (plus) sign of an unsigned integer (if a
user might want to append it later)? (Reading the comment and
implementation of `boost::detail::max_digits` I assume the answer is
yes.) Maybe, make this clear in its reference documentation.


* "boost/fixed_string/config.hpp":

- At the top of this file we have `#define
BOOST_FIXED_STRING_USE_BOOST`, which therefore makes all following
conditional definitions always use Boost equivalents of standard library
types internally.
Is it really desired to have it hard-coded here? Shouldn't the user
decide if s/he wants to use Boost compatibility types instead of the
ones coming from the standard library?

* Filenames

- I do not really like, that all files have the same name
"fixed_string.hpp". There is the main one in "boost/fixed_string",
another one in "boost/fixed_string/detail" and a third one in
"boost/fixed_string/impl".
I would prefer these files having some (at least slightly) different names.


* "CMakeLists.txt":

- This CMakeLists.txt file is written in Traditional CMake (old style
CMake) although requiring CMake 3.5.1 which knows how to handle Modern
CMake. You should consider using Modern CMake instead.

- You should leave `CMAKE_CXX_FLAGS` untouched. It is for the user of
the library, not for the developer!

- Similar, set requirements for a specific minimum required C++ version
using the `CXX_STANDARD` property instead of hard-coding it into
`CMAKE_CXX_FLAGS`.

- You should probably replace usage of `include_directories` with
`target_include_directories` on the specific targets.

- Similar for `add_definitions` and `link_directories`.

- Using `file(GLOB...)` for globbing sources is dangerous and CMake
might not always realize that some sources have changed. (see note at:
https://cmake.org/cmake/help/latest/command/file.html#glob)

- In general, I am not sure if that CMakeLists.txt file should have been
part of the review, but I recommend to provide a useful and more modern
one if this is going to become a part of Boost, too.


Best regards,
Deniz



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

Re: [FixedString] Deniz' review

Boost - Dev mailing list
On Mon, Nov 25, 2019 at 10:12 AM Deniz Bahadir via Boost
<[hidden email]> wrote:
> Besides, Boost already has some other smaller building-block libraries,
> so FixedString should be in good company.

Based on my experience with Beast, I now prefer developing new Boost
libraries to be smaller and more narrow in scope. FixedString is a
good example, as is Variant2 (I prefer Variant2 in its own library
instead of in say, Boost.Utility).

> I do not really like, that all files have the same name
> "fixed_string.hpp". There is the main one in "boost/fixed_string",
> another one in "boost/fixed_string/detail" and a third one in
> "boost/fixed_string/impl".
> I would prefer these files having some (at least slightly) different names.

Those files are private so you really shouldn't be looking at them
except in the context of a Boost review :)

There is nothing novel here, this follows Asio's naming scheme (which
I copied for Beast, and also for Boost.JSON).

> - This CMakeLists.txt file is written in Traditional CMake (old style
> CMake) although requiring CMake 3.5.1 which knows how to handle Modern
> CMake. You should consider using Modern CMake instead.

The CMakeLists.txt is for building a Visual Studio Project file only,
it is not supported for anything else. This should be mentioned in the
README.md as it is for Beast:

<https://github.com/boostorg/beast/blob/develop/README.md#visual-studio>

Regards

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

Re: [FixedString] Deniz' review

Boost - Dev mailing list
Vinnie Falco wrote:

> The CMakeLists.txt is for building a Visual Studio Project file only, it
> is not supported for anything else.

We probably need some policy on acceptable CMakeLists.txt files, which the
above isn't. A top-level CMakeLists.txt file is discoverable for both users
and globs and should meet some minimum requirements.

If you want an implementation-detail CMakeLists.txt file, full of bad
practices and only useful to you, put that in a directory somewhere.


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

Re: [FixedString] Deniz' review

Boost - Dev mailing list
On Tue, Nov 26, 2019 at 9:52 AM Peter Dimov via Boost
<[hidden email]> wrote:
> If you want an implementation-detail CMakeLists.txt file, full of bad
> practices and only useful to you, put that in a directory somewhere.

I wouldn't mind if someone submitted a pull request to make this
CMakeLists.txt work better, as long as it generated the same visual
studio solution and projects that I currently generate. Or if you want
to do it in one of your repositories such as smart_ptr (to generate a
good visual studio solution and project files) then I could copy it.

Thanks

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

Re: [FixedString] Deniz' review

Boost - Dev mailing list
Vinnie Falco wrote:
> I wouldn't mind if someone submitted a pull request to make this
> CMakeLists.txt work better, as long as it generated the same visual studio
> solution and projects that I currently generate.

I'm significantly less concerned about the file "not working better" (adding
one more library to the blacklist isn't that big of a deal) than I am of
propagating bad practices. People copy what our libraries do.


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

Re: [FixedString] Deniz' review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Tue, Nov 26, 2019 at 9:52 AM Peter Dimov via Boost
<[hidden email]> wrote:
> If you want an implementation-detail CMakeLists.txt file, full of bad
> practices and only useful to you, put that in a directory somewhere.

Hmm... no, that doesn't seem to work. I tried moving the top level
CMakeLists.txt to tools/CMakeLists.txt, and changing the calls to
add_subdirectory to:

    add_subdirectory (../bench)
    add_subdirectory (../example)
    add_subdirectory (../test)


Then from bin/ I type:

    cmake ../tools

And I get these errors:

CMake Error at CMakeLists.txt:151 (add_subdirectory):
  add_subdirectory not given a binary directory but the given source
  directory "C:/Users/vinnie/src/boost/libs/json/bench" is not a subdirectory
  of "C:/Users/vinnie/src/boost/libs/json/tools".  When specifying an
  out-of-tree source a binary directory must be explicitly specified.

Thanks

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

[FixedString] Deniz' review

Boost - Dev mailing list
> Gesendet: Dienstag, 26. November 2019 um 21:23 Uhr
> Von: "Vinnie Falco via Boost" <[hidden email]>
> An: "[hidden email] List" <[hidden email]>
> Cc: "Vinnie Falco" <[hidden email]>, "Peter Dimov" <[hidden email]>
> Betreff: Re: [boost] [FixedString] Deniz' review
> On Tue, Nov 26, 2019 at 9:52 AM Peter Dimov via Boost
> <[hidden email]> wrote:
> > If you want an implementation-detail CMakeLists.txt file, full of bad
> > practices and only useful to you, put that in a directory somewhere.
>
> Hmm... no, that doesn't seem to work. I tried moving the top level
> CMakeLists.txt to tools/CMakeLists.txt, and changing the calls to
> add_subdirectory to:
>
> add_subdirectory (../bench)
> add_subdirectory (../example)
> add_subdirectory (../test)
>
>
> Then from bin/ I type:
>
> cmake ../tools
>
> And I get these errors:
>
> CMake Error at CMakeLists.txt:151 (add_subdirectory):
> add_subdirectory not given a binary directory but the given source
> directory "C:/Users/vinnie/src/boost/libs/json/bench" is not a subdirectory
> of "C:/Users/vinnie/src/boost/libs/json/tools". When specifying an
> out-of-tree source a binary directory must be explicitly specified.

As the message states, you have to explicitly specify the binary directory:

add_subdirectory (../bench bench )
add_subdirectory (../example example)
add_subdirectory (../test test)

Those will be evaluated relative to the curreent output directory.

Best

Mike

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

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

Re: [FixedString] Deniz' review

Boost - Dev mailing list
On Tue, Nov 26, 2019 at 1:48 PM Mike via Boost <[hidden email]> wrote:
> As the message states, you have to explicitly specify the binary directory:
>
> add_subdirectory (../bench bench )
> add_subdirectory (../example example)
> add_subdirectory (../test test)

Doesn't work:

    $ cmake ../tools
    -- Selecting Windows SDK version 10.0.17763.0 to target Windows 10.0.18362.
    -- Configuring done
    CMake Error at
C:/Users/vinnie/src/boost/libs/json/bench/CMakeLists.txt:33
(add_executable):
     Cannot find source file:

        C:/Users/vinnie/src/boost/libs/json/tools/include/boost/json.hpp

.../json/tools/include/ is the wrong path, it should be
../json/include/. I tried all of these variations:

add_subdirectory (../bench ..)
add_subdirectory (../example ..)
add_subdirectory (../test ..)

add_subdirectory (../bench ../bench)
add_subdirectory (../example ../example)
add_subdirectory (../test ../test)

add_subdirectory (../bench .)
add_subdirectory (../example .)
add_subdirectory (../test .)

Maybe you could submit a pull request which moves the top level
CMakeLists.txt into tools/ ?

<https://github.com/vinniefalco/json>

Regards

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

Re: [FixedString] Deniz' review

Boost - Dev mailing list
>  At the top of this file we have `#define
BOOST_FIXED_STRING_USE_BOOST`, which therefore makes all following
conditional definitions always use Boost equivalents of standard library
types internally.

Well, it *is* the config, so that is where the user can decide on whether
the library is boost dependent or not.



On Tue, Nov 26, 2019 at 5:30 PM Vinnie Falco via Boost <
[hidden email]> wrote:

> On Tue, Nov 26, 2019 at 1:48 PM Mike via Boost <[hidden email]>
> wrote:
> > As the message states, you have to explicitly specify the binary
> directory:
> >
> > add_subdirectory (../bench bench )
> > add_subdirectory (../example example)
> > add_subdirectory (../test test)
>
> Doesn't work:
>
>     $ cmake ../tools
>     -- Selecting Windows SDK version 10.0.17763.0 to target Windows
> 10.0.18362.
>     -- Configuring done
>     CMake Error at
> C:/Users/vinnie/src/boost/libs/json/bench/CMakeLists.txt:33
> (add_executable):
>      Cannot find source file:
>
>         C:/Users/vinnie/src/boost/libs/json/tools/include/boost/json.hpp
>
> .../json/tools/include/ is the wrong path, it should be
> ../json/include/. I tried all of these variations:
>
> add_subdirectory (../bench ..)
> add_subdirectory (../example ..)
> add_subdirectory (../test ..)
>
> add_subdirectory (../bench ../bench)
> add_subdirectory (../example ../example)
> add_subdirectory (../test ../test)
>
> add_subdirectory (../bench .)
> add_subdirectory (../example .)
> add_subdirectory (../test .)
>
> Maybe you could submit a pull request which moves the top level
> CMakeLists.txt into tools/ ?
>
> <https://github.com/vinniefalco/json>
>
> Regards
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>

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

Re: [FixedString] Deniz' review

Boost - Dev mailing list
On 27/11/2019 18:02, Krystian Stasiowski wrote:
>>   At the top of this file we have `#define
>> BOOST_FIXED_STRING_USE_BOOST`, which therefore makes all following
>> conditional definitions always use Boost equivalents of standard library
>> types internally.
>
> Well, it *is* the config, so that is where the user can decide on whether
> the library is boost dependent or not.

Users of Boost libraries are not expected to modify Boost source files,
including header files, and in particular including config.hpp header files.

Those files are supposed to be used to detect and translate "external"
configuration options (either defined by the user before including the
library or from Boost.Predef or Boost.Config) to "internal" ones used
within the library.

Unconditionally defining something in a config.hpp is therefore usually
incorrect.



Also, you appear to be replying to posts in the thread different from
the ones that you are quoting, which makes following the chain in a
threaded email client tricky.

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

Re: [FixedString] Deniz' review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Tue, Nov 26, 2019 at 9:04 PM Krystian Stasiowski via Boost <
[hidden email]> wrote:

> >  At the top of this file we have `#define
> BOOST_FIXED_STRING_USE_BOOST`


This macro should be named BOOST_FIXED_STRING_STANDALONE and should not be
defined anywhere in the library, it is for users to define if they want to
use the library without boost. The default configuration should be to use
boost. Without boost, the library will require at least C++17, for
string_view.

--
Regards,
Vinnie

Follow me on GitHub: https://github.com/vinniefalco

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