[review][fixed_string] End of review period approaching and a plea

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

[review][fixed_string] End of review period approaching and a plea

Boost - Dev mailing list
The review period for candidate Boost.FixedString finishes tomorrow, Dec 4.

There's an ongoing lively discussion on many aspects of the library,
which is very good,
but only three of you have cast your acceptance/rejection vote, which is
not good.

This is a gentle petition to the people engaging in the review who
haven't voted yet:

   Vinícius, Nevin, Mike, Andrzej, JeanHeyd, Hans, Peter, Julien,
Andrey, Tim, Gavin, Emil,

(and anyone who might be still lurking) to please submit a review with
an explicit vote.
If you feel like you need some more time to do the work, I think we are
in a position to
extend the review period, but please express your intentions before I
ask the review
wizards.

Thank you!

Joaquín M López Muñoz
Review manager for candidate Boost.FixedString


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

Re: [review][fixed_string] End of review period approaching and a plea

Boost - Dev mailing list
  Hi all,

Thank you all for participating in the review so far. I'd like to outline
some of the changes that have been implemented thus far, and others that
are pending as there is not a clear consensus. The live branch can be found
at https://github.com/18/fixed_string/tree/review-patch

- Empty class specialization for N = 0: Implemented
- Use of smallest possible type to store size: Implemented
- More noexcept: Mostly implemented, still need to address exception
specification for template overloads intended for `string_view`
- Hash support: Implemented for both std::hash and boost::hash
- Change `substr` to return a `fixed_string` and add a `subview` function:
Implemented
- Fixed the user configuration macro and changed the name to
BOOST_FIXED_STRING_STANDALONE
- Added mandatory meta folder

A few things on the list of change remain:
- `constexpr` all the things!... Only C++20 allows for members to remain
uninitialized by a constexpr constructor, so use in previous revisions of
the standard would require initialization of each element, which could
impose an unacceptable performance penalty
- We still have not settled on a name that is concise and descriptive
- No consensus has been reached on how operator+ should be implemented
- Documentation needs to be updated

Thank you,
Krystian Stasiowski


On Tue, Dec 3, 2019 at 3:11 PM Joaquin M López Muñoz via Boost <
[hidden email]> wrote:

> The review period for candidate Boost.FixedString finishes tomorrow, Dec 4.
>
> There's an ongoing lively discussion on many aspects of the library,
> which is very good,
> but only three of you have cast your acceptance/rejection vote, which is
> not good.
>
> This is a gentle petition to the people engaging in the review who
> haven't voted yet:
>
>    Vinícius, Nevin, Mike, Andrzej, JeanHeyd, Hans, Peter, Julien,
> Andrey, Tim, Gavin, Emil,
>
> (and anyone who might be still lurking) to please submit a review with
> an explicit vote.
> If you feel like you need some more time to do the work, I think we are
> in a position to
> extend the review period, but please express your intentions before I
> ask the review
> wizards.
>
> Thank you!
>
> Joaquín M López Muñoz
> Review manager for candidate Boost.FixedString
>
>
> _______________________________________________
> 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: [review][fixed_string] End of review period approaching and a plea

Boost - Dev mailing list


> On 4 Dec 2019, at 05:34, Krystian Stasiowski via Boost <[hidden email]> wrote:
>
>
> - We still have not settled on a name that is concise and descriptive

Maybe name should emphasize storage placement, rather than capacity fixedness.

inplace_string


bjørn



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

Re: [review][fixed_string] End of review period approaching and a plea

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

> - Empty class specialization for N = 0: Implemented

I don't like this change. A special case for close to zero benefit that
changes the semantics of data() to not be unique per instance.

Storing the size (as capacity - size) in the last char for N < 256 will have
more impact, but I'm not sure that it too is worth the added complexity.


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

Re: [review][fixed_string] End of review period approaching and a plea

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
> We still have not settled on a name that is concise and descriptive

How about capped_string, perhaps too British but it does mean with a fixed capacity.


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

Re: [review][fixed_string] End of review period approaching and a plea

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Wed, 4 Dec 2019 at 01:41, Peter Dimov via Boost <[hidden email]>
wrote:

> Krystian Stasiowski wrote:
>
> > - Empty class specialization for N = 0: Implemented
>
> I don't like this change. A special case for close to zero benefit that
> changes the semantics of data() to not be unique per instance.
>

+1.

Storing the size (as capacity - size) in the last char for N < 256 will
> have
> more impact, but I'm not sure that it too is worth the added complexity.
>
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>


--
@realdegski
https://brave.com/google-gdpr-workaround/
"We value your privacy, click here!" Sod off! - degski
"Anyone who believes that exponential growth can go on forever in a finite
world is either a madman or an economist" - Kenneth E. Boulding
"Growth for the sake of growth is the ideology of the cancer cell" - Edward
P. Abbey

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

Re: [review][fixed_string] End of review period approaching and a plea

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Tue, Dec 3, 2019 at 11:42 PM Peter Dimov via Boost
<[hidden email]> wrote:
> I don't like this change. A special case for close to zero benefit that
> changes the semantics of data() to not be unique per instance.

I don't feel strongly about it either way, but since when is data()
guaranteed to be unique per instance?

Thanks

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

Re: [review][fixed_string] End of review period approaching and a plea

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
[hidden email] wrote:
> only three of you have cast your acceptance/rejection vote

Reviews aren't votes!





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

Re: [review][fixed_string] End of review period approaching and a plea

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Peter Dimov <[hidden email]> wrote:

> Krystian Stasiowski wrote:
>
>> - Empty class specialization for N = 0: Implemented
>
> I don't like this change. A special case for close to zero benefit that
> changes the semantics of data() to not be unique per instance.

I would hope to see close to the same semantics as std::array<T,0>,
which I believe allows data() to return nullptr.

> Storing the size (as capacity - size) in the last char for N < 256 will have
> more impact, but I'm not sure that it too is worth the added complexity.

Why the last char, rather than always having the size
(of whatever appropriate type) first?  Is the idea that
this makes data() and c_str() essentially no-ops?  I guess
the benefit of that depends on how often you need empty()
or size() vs. data() or c_str().  Or is there some other
issue?


Regards, Phil.







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

Re: [review][fixed_string] End of review period approaching and a plea

Boost - Dev mailing list

> I would hope to see close to the same semantics as std::array<T,0>,
> which I believe allows data() to return nullptr.

I don't think so:

"There is a special case for a zero-length array (|N == 0|). In that
case, array.begin() == array.end(), which is some unique value. The
effect of calling front() or back() on a zero-sized array is undefined."
from https://en.cppreference.com/w/cpp/container/array

IMO this excludes nullptr as that won't be unique



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

smime.p7s (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [review][fixed_string] End of review period approaching and a plea

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
El 04/12/2019 a las 15:35, Phil Endecott via Boost escribió:
> [hidden email] wrote:
>> only three of you have cast your acceptance/rejection vote
>
> Reviews aren't votes!


Absolutely. But proper reviews must include the vote, just as you've done.

Thank you,

Joaquín M López Muñoz
Review manager for candidate Boost.FixedString


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

Re: [review][fixed_string] End of review period approaching and a plea

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Vinnie Falco wrote:
> On Tue, Dec 3, 2019 at 11:42 PM Peter Dimov via Boost
> <[hidden email]> wrote:
> > I don't like this change. A special case for close to zero benefit that
> > changes the semantics of data() to not be unique per instance.
>
> I don't feel strongly about it either way, but since when is data()
> guaranteed to be unique per instance?

This is a property of the implementation that is no longer true after the
change. It may not have been "guaranteed" but it was true.


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

Re: [review][fixed_string] End of review periodapproaching and a plea

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Phil Endecott wrote:

> Peter Dimov <[hidden email]> wrote:
>
> > Krystian Stasiowski wrote:
> >
> >> - Empty class specialization for N = 0: Implemented
> >
> > I don't like this change. A special case for close to zero benefit that
> > changes the semantics of data() to not be unique per instance.
>
> I would hope to see close to the same semantics as std::array<T,0>, which
> I believe allows data() to return nullptr.

nullptr is invalid here, because the string is null-terminated.

> > Storing the size (as capacity - size) in the last char for N < 256 will
> > have more impact, but I'm not sure that it too is worth the added
> > complexity.
>
> Why the last char, rather than always having the size (of whatever
> appropriate type) first?  Is the idea that this makes data() and c_str()
> essentially no-ops?

The idea here is that you win one byte by reusing the last byte of the
storage as the size, overlapping it with the null terminator in the size()
== N case (because capacity - size becomes 0).


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

Re: [review][fixed_string] End of review periodapproaching and a plea

Boost - Dev mailing list
On 2019-12-04 19:05, Peter Dimov via Boost wrote:

> Phil Endecott wrote:
>> Peter Dimov <[hidden email]> wrote:
>
>> > Storing the size (as capacity - size) in the last char for N < 256
>> will > have more impact, but I'm not sure that it too is worth the
>> added > complexity.
>>
>> Why the last char, rather than always having the size (of whatever
>> appropriate type) first?  Is the idea that this makes data() and
>> c_str() essentially no-ops?
>
> The idea here is that you win one byte by reusing the last byte of the
> storage as the size, overlapping it with the null terminator in the
> size() == N case (because capacity - size becomes 0).

I'm not sure this would actually be beneficial in terms if performance.
Ignoring the fact that size() becomes more expensive, and this is a
relatively often used function, you also have to access the tail of the
storage, which is likely on a different cache line than the beginning of
the string. It is more likely that the user will want to process the
string in the forward direction, possibly not until the end (think
comparison operators, copy/assignment, for instance). If the string is
not close to full capacity, you would only fetch the tail cache line to
get the string size.

It is for this reason placing any auxiliary members like size is
preferable before the storage array. Of course, if you prefer memory
size over speed, placing size in the last byte is preferable.

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

Re: [review][fixed_string] End of review periodapproaching and a plea

Boost - Dev mailing list
Andrey Semashev wrote:
> > The idea here is that you win one byte by reusing the last byte of the
> > storage as the size, overlapping it with the null terminator in the
> > size() == N case (because capacity - size becomes 0).
>
> I'm not sure this would actually be beneficial in terms if performance.
> Ignoring the fact that size() becomes more expensive, and this is a
> relatively often used function, you also have to access the tail of the
> storage, which is likely on a different cache line than the beginning of
> the string.

Yes, probably. As I said, it's not clear that we should bother with it (I
wouldn't), but it at least gives consistent space savings for all short
strings.

That said, one could use it for N < 32, when the size will be in the same
cache line.


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

Re: [review][fixed_string] End of review period approaching and a plea

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 2019-12-04 18:24, Alexander Grund via Boost wrote:

>
>> I would hope to see close to the same semantics as std::array<T,0>,
>> which I believe allows data() to return nullptr.
>
> I don't think so:
>
> "There is a special case for a zero-length array (|N == 0|). In that
> case, array.begin() == array.end(), which is some unique value. The
> effect of calling front() or back() on a zero-sized array is undefined."
> from https://en.cppreference.com/w/cpp/container/array
>
> IMO this excludes nullptr as that won't be unique

I don't think iterators from different instances of a container are
comparable. IOW, "unique" means distinct from any possible values of
valid iterators obtained from this particular container instance.

That might not apply to pointers, though. I don't remember whether there
are any guarantees wrt. data() of a zero-sized array, for example.

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

Re: [review][fixed_string] End of review period approaching and a plea

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 2019-12-04 19:01, Peter Dimov via Boost wrote:

> Vinnie Falco wrote:
>> On Tue, Dec 3, 2019 at 11:42 PM Peter Dimov via Boost
>> <[hidden email]> wrote:
>> > I don't like this change. A special case for close to zero benefit
>> that > changes the semantics of data() to not be unique per instance.
>>
>> I don't feel strongly about it either way, but since when is data()
>> guaranteed to be unique per instance?
>
> This is a property of the implementation that is no longer true after
> the change. It may not have been "guaranteed" but it was true.

I don't think it should be guaranteed.

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

Re: [review][fixed_string] End of review period approaching and a plea

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Andrey Semashev wrote:

> On 2019-12-04 18:24, Alexander Grund via Boost wrote:
> >
> >> I would hope to see close to the same semantics as std::array<T,0>,
> >> which I believe allows data() to return nullptr.
> >
> > I don't think so:
> >
> > "There is a special case for a zero-length array (|N == 0|). In that
> > case, array.begin() == array.end(), which is some unique value. The
> > effect of calling front() or back() on a zero-sized array is undefined."
> > from https://en.cppreference.com/w/cpp/container/array
> >
> > IMO this excludes nullptr as that won't be unique
>
> I don't think iterators from different instances of a container are
> comparable. IOW, "unique" means distinct from any possible values of valid
> iterators obtained from this particular container instance.
>
> That might not apply to pointers, though.

Pointers are comparable with ==, and array<T, 0> usually returns
reinterpret_cast<T*>(this) from data() to satisfy the uniqueness
requirement.

Or used to return - I see that libstdc++ returns nullptr now, probably
because reinterpret_cast can't be constexpr.

https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/array#L76 


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

Re: [review][fixed_string] End of review period approaching and a plea

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Andrey Semashev wrote:

> I don't think it should be guaranteed.

I honestly see no reason for it not to be. The difference between a one-byte
class and a zero-byte class is only relevant on a few occasions, which
rarely occur for fixed_string. Unnecessary lack of consistence is a clear
downside.


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

Re: [review][fixed_string] End of review period approaching and a plea

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Wed, Dec 4, 2019 at 1:50 PM Peter Dimov via Boost
<[hidden email]> wrote:

>
> Andrey Semashev wrote:
> > On 2019-12-04 18:24, Alexander Grund via Boost wrote:
> > >
> > >> I would hope to see close to the same semantics as std::array<T,0>,
> > >> which I believe allows data() to return nullptr.
> > >
> > > I don't think so:
> > >
> > > "There is a special case for a zero-length array (|N == 0|). In that
> > > case, array.begin() == array.end(), which is some unique value. The
> > > effect of calling front() or back() on a zero-sized array is undefined."
> > > from https://en.cppreference.com/w/cpp/container/array
> > >
> > > IMO this excludes nullptr as that won't be unique
> >
> > I don't think iterators from different instances of a container are
> > comparable. IOW, "unique" means distinct from any possible values of valid
> > iterators obtained from this particular container instance.
> >
> > That might not apply to pointers, though.
>
> Pointers are comparable with ==, and array<T, 0> usually returns
> reinterpret_cast<T*>(this) from data() to satisfy the uniqueness
> requirement.
>
> Or used to return - I see that libstdc++ returns nullptr now, probably
> because reinterpret_cast can't be constexpr.
>
> https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/std/array#L76

Precisely for this reason, but also the uniqueness requirement was on
`begin()` and `end()` and not `.data()`:

     "In the case that N == 0, begin() == end() == unique value. The
return value of data() is unspecified."

     -- Working Draft, http://eel.is/c++draft/array#zero-2

begin() and end() are already restricted library-wide to not be
comparable with begin() and end() -- let alone any iterator -- from a
container that does not generate them, so cross-container uniqueness
has never been a thing. There is an issue open to generate better
wording around this to make it more clear:
https://cplusplus.github.io/LWG/lwg-active.html#2157

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