[review] Review of PolyCollection starts today (May 3rd)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
53 messages Options
123
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
El 16/05/2017 a las 16:20, Thorsten Ottosen via Boost escribió:

> Den 15-05-2017 kl. 21:21 skrev Joaquin M López Muñoz via Boost:
>
>> I think this variation of a) might perform better (code untested):
>>
>>    template<typename Model,typename Allocator>
>>    bool operator==(
>>      const poly_collection<Model,Allocator>& x,
>>      const poly_collection<Model,Allocator>& y)
>>    {
>>      typename poly_collection<Model,Allocator>::size_type s=0;
>>      const auto &mapx=x.map,&mapy=y.map;
>>      for(const auto& p:mapx){
>>        s+=p.second.size();
>>        auto it=mapy.find(p.first);
>> if(it==mapy.end()?!p.second.empty():p.second!=it->second)return false;
>>      }
>>      if(s!=mapy.size())return false;
>>      return true;
>>    }
>
>
> I not sure about this version. mapy.size() should be y.size() ?!?

My typo, it should read as you say:

   template<typename Model,typename Allocator>
   bool operator==(
     const poly_collection<Model,Allocator>& x,
     const poly_collection<Model,Allocator>& y)
   {
     typename poly_collection<Model,Allocator>::size_type s=0;
     const auto &mapx=x.map,&mapy=y.map;
     for(const auto& p:mapx){
       s+=p.second.size();
       auto it=mapy.find(p.first);
if(it==mapy.end()?!p.second.empty():p.second!=it->second)return false;
     }
     if(s!=y.size())return false;
     return true;
   }

> Or do you need sx and sy? But also, once all the segments have been
> compared in the loop, you know the x.size() == y.size() ...

Not so: consider the case where y has a segment (let's call it seg) which is
not present in x; in this case the for loop would pass succesfully and seg
wouldn't have been hit, and we'd thus erroneously return true. That's
why we need the extra check on size. In other words, we can't determine
equality by only going through x's segments.

Joaquín M López Muñoz

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

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
Den 16-05-2017 kl. 19:41 skrev Joaquin M López Muñoz via Boost:
> El 16/05/2017 a las 16:20, Thorsten Ottosen via Boost escribió:


>> Or do you need sx and sy? But also, once all the segments have been
>> compared in the loop, you know the x.size() == y.size() ...
>
> Not so: consider the case where y has a segment (let's call it seg)
> which is
> not present in x; in this case the for loop would pass succesfully and seg
> wouldn't have been hit, and we'd thus erroneously return true. That's
> why we need the extra check on size. In other words, we can't determine
> equality by only going through x's segments.

right.

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

Re: [review] Review of PolyCollection starts today (May 3rd)

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

> Please post your comments and review to the boost mailing list (preferably), the Boost Library Incubator. or privately to the Review Manager (to me ;-). Here are some questions you might want to answer in your review:
>
> - What is your evaluation of the design?

The overall design seems sound.  The library identifies a clear need and seems to provide a carefully crafted solution.

> - What is your evaluation of the implementation?

I did not look into the implementation in detail.

> - What is your evaluation of the documentation?

The documentation seemed amazingly clear, even though I am not a game developer and do not relate daily to warriors and goblins.  The common thread in the documentation and the clarity of progression of the tutorial was great.  I did not review the reference documentation, though.  In part, it seems unnecessary because the basic concepts seemed so well thought out and clearly described.  I appreciate that there may be subtleties to bits of the API, but I will leave it to those more familiar with the details of implementation to improve.

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

I feel this library would be very useful and can see using it in my own code.  For example, right now I am working with a hierarchy of objects that must be stored in a container; there are lots of objects but relatively few classes in the hierarchy.  This seems perfect for the base_collection container.

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

I did use the library.  In particular, I compiled all of the examples and built a small program with a class hierarchy of my own to test my understanding of the base_collection container.  This was done with Apple LLVM version 8.0.0 (clang-800.0.42.1).  The only problem was that the following test cases needed -std=c++14 (rather than c++11) because std::make_unique is not defined for c++11 on that compiler: algorithms, basic_function, and segmented_structure.

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

I have read through most of the documentation (basically all of the tutorial) and have been following much of the discussion during the review.

> - Are you knowledgeable about the problem domain?

I am not particularly knowledgeable about writing optimized collections such as this, but I am knowledgeable from a user perspective on using them and on some of the optimizations that would be useful.

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

Yes, I feel this would be a very useful addition to Boost and would likely use it in my code.

Cheers,
Brook


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

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
El 17/05/2017 a las 17:12, Brook Milligan via Boost escribió:
> [...]
>
>> - What is your evaluation of the documentation?
> The documentation seemed amazingly clear, even though I am not a game developer and do not relate daily to warriors and goblins. [...]

Neither do I. I suspect real game developers will smile at the naïveté
of the example,
though.

> [...]
> - Did you try to use the library? With what compiler? Did you have any problems?
> I did use the library.  In particular, I compiled all of the examples and built a small program with a class hierarchy of my own to test my understanding of the base_collection container.  This was done with Apple LLVM version 8.0.0 (clang-800.0.42.1).  The only problem was that the following test cases needed -std=c++14 (rather than c++11) because std::make_unique is not defined for c++11 on that compiler: algorithms, basic_function, and segmented_structure.

Thanks for noticing this, will fix in the corresponding Jamvile.v2. By
the way, algorithms.cpp
need C++14 not because of make_unique (which is not used there) but for
generic lambdas
such as:

https://github.com/joaquintides/poly_collection/blob/master/example/algorithms.cpp#L152-L155

FWIW, algorithms is already marked as needing C++14:

https://github.com/joaquintides/poly_collection/blob/master/example/Jamfile.v2#L15-L19

so I guess you didn't use b2 for building for went directly with the
compiler command line,
right?

> [...]
>
>> - Do you think the library should be accepted as a Boost library?
> Yes, I feel this would be a very useful addition to Boost and would likely use it in my code.

Thank you!

Joaquín M López Muñoz

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

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list

> On May 17, 2017, at 12:12 PM, Joaquin M López Muñoz via Boost <[hidden email]> wrote:
>
> Thanks for noticing this, will fix in the corresponding Jamvile.v2. By the way, algorithms.cpp
> need C++14 not because of make_unique (which is not used there) but for generic lambdas
> such as:
>
> https://github.com/joaquintides/poly_collection/blob/master/example/algorithms.cpp#L152-L155

Yes, I forgot the details at the time, but this reminds me that generic lambdas were exactly the problem.

> FWIW, algorithms is already marked as needing C++14:
>
> https://github.com/joaquintides/poly_collection/blob/master/example/Jamfile.v2#L15-L19
>
> so I guess you didn't use b2 for building for went directly with the compiler command line,
> right?

Yes, that is correct.  I personally, don’t use b2 for building anything except boost itself.  It seems too far afield from “normal” build environments to be useful in my own work.  I was not really pointing this out as a problem with the library, but rather that I thought there was some mention of it being a c++11 library and then I found I need to use c++14 at some points.  I suppose that is entirely due to the example code, though, and not the library itself.  Perhaps a mention of the need for c++14 in some examples?  

None of this is a big deal, though.  The bottom line is it seems to be a great piece of work.

Cheers,
Brook


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

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
AMDG

I vote to ACCEPT PolyCollection into Boost.

Details:

General:

Missing .gitattributes

index.html:

No comments.

an_efficient_polymorphic_data_st.html:

"interspersedly"
This word sounds a bit awkward to me.

"the information is typically available at compile
time in the point of insertion in the vector"
I would say "at the point" instead of "in the point"

tutorial.html:

"Imagine we are developing a role game in C++"
Do you mean "role playing game"?

For choosing which type to insert, std::discrete_distribution
is slightly more appropriate than std::uniform_real_distribution.

"...by the user code --we will see more about this later"
This should be an em-dash.

"Insertion mechanisms are rather..."
-> "The insertion mechanisms are rather..."

"to explicitly forbid"
split infinitive

"...it is possible to only address the elements of a given
segment ..."
Address here is a bit confusing as "address" has a specific
meaning in programming.  Access is probably a better term
to use.

reference.html:

"...in the same order, and viceversa."
s/viceversa/vice versa/

"cc.empty(index)...
Throws: If the indicated type is not registered. "
Does this really make sense?  I think it would be
more useful to treat non-registered segments as
empty.  Throwing here is somewhat inconsistent
with operator== which considers non-registered
segments to be equivalent to empty segments.

"cc.max_size()
REVIEW THIS DESIGN DECISION"
"the minimum return value of max_size for each of the segments of the
collection"
I think that the fundamental invariant of
max_size should be that size() <= max_size().
Both max_size and capacity are a bit strange.
My inclination would be to remove capacity
entirely (or rename it) and change max_size to
mean the maximum possible size of the whole container.

Class template base_collection
"subobject(x) = static_cast<Derived&>(x) with typeid(x)==typeid(Derived)"
static_cast excludes virtual inhertance, which I presume is unsupported.

algorithm.hpp:

61:
segment_split: ADL (Contrived test case in test_adl1.cpp)

any_collection.hpp:

"See http://www.boost.org/libs/any_collection for library home page."
s/any_collection/poly_collection/

operator!= should be defined in the same way as operator==.
Overload resolution is subtly different for a
non-template friend function defined inline vs.
a regular free function template.  This is
especially problematic because any_collection_fwd.hpp
declares a different operator== which is not implemented.

any_collection_fwd.hpp:

http://www.boost.org/libs/any_collection again.

base_collection/function_collection have identical issues.

exception.hpp:

No comments.

detail/any_iterator.hpp:

line 74:  explicit operator Concrete*()const noexcept
This really should have an assert that the type is correct.

detail/any_model.hpp:

line 53:
!type_erasure::is_subconcept<type_erasure::assignable<>,Concept2>::value
Boost.TypeErasure was recently extended to support move
assignment (assignable<_self, _self&&>).  Should this
also be handled here?  For the record, I'm planning
to fix the constructors and assignment operators of
any so that is_constructible and is_assignable, etc.
work, but it's quite annoying to implement.

line 66, 94, 112, 123:
    type_erasure::is_subconcept<type_erasure::typeid_<>,Concept>::value
This isn't quite correct.  You need to check
typeid_<remove_cv_t<remove_reference_t<T>>>
Also it looks like this code is trying to allow an any
that doesn't use typeid_ to be stored in a segment
of anys (at least that's the impression that I get from
the definition of subobject(x) for any_collection in
reference.html).  Trying to do this will error out in
split_segment.hpp:303 in build_index when constructing
the value_type.

auto_iterator.hpp:

No comments.

base_model.hpp:

line 45: static void* subaddress(T& x){return boost::addressof(x);}
This assumes that the address of the base is the address
of the whole object which is wrong and can fail in the
case of multiple inheritance.  You need to use dynamic_cast<void*>.
(Failing test case included: test_multiple_inheritance.cpp)

callable_wrapper.hpp:

I'm somewhat curious about the rationale for
using callable_wrapper instead of std::function w/ std::ref.
Ah, I see:
- data() is needed by function_model::subaddress
- std::function can waste a lot of memory, depending on the implementation.

line 83:        return r(std::forward<Args>(args)...);
This fails to handle R = void correctly.  The return
value of the function should be ignored.
(Failing test case included: test_void_return.cpp)

callable_wrapper_iterator.hpp:

function_model.hpp:

No comments.

functional.hpp:

No comments.

integer_sequence.hpp:

This can't possibly be the best implementation.
make_index_sequence<N> and make_index_sequence<M>
with N != M generate N+M totally independent
instantiations of make_int_seq_impl.

From the source:
"...but make_index_sequence is not hard to
implement *(if efficiency is not a concern)*"
[emphasis added]

is_acceptible.hpp, is_callable.hpp, is_constructible.hpp,
is_final.hpp, is_nothrow_eq_comparable.hpp, iterator_impl.hpp,
iterator_traits.hpp, packed_segment.hpp:

No comments.

poly_collection.hpp:

If poly_collection is defined in namespace detail,
that means that your internal functions can be
found by ADL for user defined types that mention
poly_collection.  (I always avoid putting types
that are part of the public interface in namespace
detail to avoid this problem.)  Test case in test_adl2.cpp

line 1109: "// TODO: can we do better than two passes?"
This comment seems obsolete.

segment.hpp, segment_backend.hpp, split_segment.hpp,
stride_iterator.hpp, type_restitution.hpp, value_holder.hpp

No comments.

In Christ,
Steven Watanabe



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

test_adl1.cpp (339 bytes) Download Attachment
test_void_return.cpp (170 bytes) Download Attachment
test_multiple_inheritance.cpp (582 bytes) Download Attachment
test_adl2.cpp (386 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
El 17/05/2017 a las 21:37, Brook Milligan escribió:
> [...] I was not really pointing this out as a problem with the
> library, but rather that I thought there was some mention of it being
> a c++11 library and then I found I need to use c++14 at some points. I
> suppose that is entirely due to the example code, though, and not the
> library itself.

Correct, the library itself is strictly C++11. I decided to use C++14 at
the examples when convenient
so as not to divert readers' attention with workarounds etc; this is
particularly the case with
generic lambdas.

> Perhaps a mention of the need for c++14 in some examples?

Yes, I can do that.

Joaquín M López Muñoz

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

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
El 18/05/2017 a las 2:59, Steven Watanabe via Boost escribió:
> AMDG
>
> I vote to ACCEPT PolyCollection into Boost.

Thank you Steven! Sorry it took me so long to answer your post, got a busy
weekend and yours is not a review one can deal with in a hurry.

> Details:
>
> General:
>
> Missing .gitattributes

Noted. By the way, I couldn't find this as a requirement in

http://www.boost.org/development/requirements.html

> [...]
>
> an_efficient_polymorphic_data_st.html:
>
> "interspersedly"
> This word sounds a bit awkward to me.

Any less awkward synonim? For other readers' convenience, the statement in
which the word appears is:

"This mechanism is rendered mostly useless when derived1, derived2 and
derived3 elements are traversed interspersedly without a definite pattern."

> "the information is typically available at compile
> time in the point of insertion in the vector"
> I would say "at the point" instead of "in the point"

You're right, noted.

> tutorial.html:
>
> "Imagine we are developing a role game in C++"
> Do you mean "role playing game"?

Yep, will change that.

> For choosing which type to insert, std::discrete_distribution
> is slightly more appropriate than std::uniform_real_distribution.

Your're right, will switch to std::discrete_distribution.

> "...by the user code --we will see more about this later"
> This should be an em-dash.

Noted.

> "Insertion mechanisms are rather..."
> -> "The insertion mechanisms are rather..."

Not being a native English speaker (nor Russian either, who typically have
a hard time with definite articles :-) ), "Insertion mechanisms" looks
perfectly
OK to me, but I can change that anyway.

> "to explicitly forbid"
> split infinitive

At the risk of starting a style war, it is my understanding that split
infinitives are
okay when used wth caution. Here, the alternative:

"to forbid explicitly copying at"

seems (to me) less clear as it might be parsed as referring to some sort of
explicit copying... By the way, there are other instances of split
infinitive in this
section, for instance "to only address" (mentioned below).

> "...it is possible to only address the elements of a given
> segment ..."
> Address here is a bit confusing as "address" has a specific
> meaning in programming.  Access is probably a better term
> to use.

Will change that.

> reference.html:
>
> "...in the same order, and viceversa."
> s/viceversa/vice versa/

Noted. It is "viceversa" in Spanish, hence the mistake.

> "cc.empty(index)...
> Throws: If the indicated type is not registered. "
> Does this really make sense?  I think it would be
> more useful to treat non-registered segments as
> empty.  Throwing here is somewhat inconsistent
> with operator== which considers non-registered
> segments to be equivalent to empty segments.

Non-registered == empty would lead to stranger situations, IMHO: we
can't hold the invariant that empty(index)==(begin(index)==end(index)) as
begin/end(index) cannot return anything sensible for non-registered
segments. Similarly, max_size, capacity etc. need to forward to the
underlying segment to return something, unless we make up the return
values for non-registered segments.

> "cc.max_size()
> REVIEW THIS DESIGN DECISION"
> "the minimum return value of max_size for each of the segments of the
> collection"
> I think that the fundamental invariant of
> max_size should be that size() <= max_size().
> Both max_size and capacity are a bit strange.
> My inclination would be to remove capacity
> entirely (or rename it) and change max_size to
> mean the maximum possible size of the whole container.

This is a design area I'm definitely not happy with. Please follow this
thread
for a related discussion:

https://lists.boost.org/Archives/boost/2016/11/231784.php

I'm leaning towards dropping (global) max_size/capacity, as you and Thorsten
suggest, till we find sensible semantics for them.

> Class template base_collection
> "subobject(x) = static_cast<Derived&>(x) with typeid(x)==typeid(Derived)"
> static_cast excludes virtual inhertance, which I presume is unsupported.

Ummm.. Hadn't thought about virtual inheritance. At first blush, seems
like virtual
inheritance would be automatically supported if only we fix subaddress as
you suggest below, don't you think?

(the "static_cast<Derived&>(x)" you refer to is only shown for
explanatory purposes
and does not appear anywhere in the actual code, as you probably guessed).

> algorithm.hpp:
>
> 61:
> segment_split: ADL (Contrived test case in test_adl1.cpp)

Noted. Please enlighten me: is it OK to qualify the offending call as
detail::segment_split, or should I fully qualify as
::boost::poly_collection::detail::segment_split?

> any_collection.hpp:
>
> "See http://www.boost.org/libs/any_collection for library home page."
> s/any_collection/poly_collection/

Right, will change it.

> operator!= should be defined in the same way as operator==.
> Overload resolution is subtly different for a
> non-template friend function defined inline vs.
> a regular free function template.  This is
> especially problematic because any_collection_fwd.hpp
> declares a different operator== which is not implemented.

You're absolutely right, will fix it.

> any_collection_fwd.hpp:
>
> http://www.boost.org/libs/any_collection again.
>
> base_collection/function_collection have identical issues.

Noted again.

> [...]
>
> detail/any_iterator.hpp:
>
> line 74:  explicit operator Concrete*()const noexcept
> This really should have an assert that the type is correct.

I don't see the need as any_iterator is a detail class not directly
exposed to
user code.

> detail/any_model.hpp:
>
> line 53:
> !type_erasure::is_subconcept<type_erasure::assignable<>,Concept2>::value
> Boost.TypeErasure was recently extended to support move
> assignment (assignable<_self, _self&&>).  Should this
> also be handled here?

Right, in fact now that move assignment is provided (didn't notice that)
we should
check this rather than copy assignment.

> For the record, I'm planning
> to fix the constructors and assignment operators of
> any so that is_constructible and is_assignable, etc.
> work, but it's quite annoying to implement.

That'd be a boon.

> line 66, 94, 112, 123:
>      type_erasure::is_subconcept<type_erasure::typeid_<>,Concept>::value
> This isn't quite correct.  You need to check
> typeid_<remove_cv_t<remove_reference_t<T>>>

Noted, will change it.

> Also it looks like this code is trying to allow an any
> that doesn't use typeid_ to be stored in a segment
> of anys (at least that's the impression that I get from
> the definition of subobject(x) for any_collection in
> reference.html).

That's the intention. If I can't get the wrapped object I'll store the any
objects at least.

> Trying to do this will error out in
> split_segment.hpp:303 in build_index when constructing
> the value_type.

I'm not seeing this. First of all, there's a bug in

https://github.com/joaquintides/poly_collection/blob/master/include/boost/poly_collection/detail/any_model.hpp#L128

s/any_cast<void*>/any_cast<const void*>. Once you do that, the following
compiles fine:

   #include <boost/poly_collection/any_collection.hpp>

   int main()
   {
     using concept=boost::mpl::vector<
       boost::type_erasure::copy_constructible<>,
       boost::type_erasure::relaxed
     >;
     boost::any_collection<concept> c;
     auto a=boost::type_erasure::any<concept>{0};
     c.insert(a);
   }

> [...]
>
> base_model.hpp:
>
> line 45: static void* subaddress(T& x){return boost::addressof(x);}
> This assumes that the address of the base is the address
> of the whole object which is wrong and can fail in the
> case of multiple inheritance.  You need to use dynamic_cast<void*>.
> (Failing test case included: test_multiple_inheritance.cpp)

Absolutely, will change it. See also my comment above re
"static_cast<Derived&>(x)".

> callable_wrapper.hpp:
>
> [...]
>
> line 83:        return r(std::forward<Args>(args)...);
> This fails to handle R = void correctly.  The return
> value of the function should be ignored.
> (Failing test case included: test_void_return.cpp)

Got it. I guess I only need to

s/return r(std::forward<Args>(args)...)/return
static_cast<R>(r(std::forward<Args>(args)...))

right?

> [...]
>
> integer_sequence.hpp:
>
> This can't possibly be the best implementation.
> make_index_sequence<N> and make_index_sequence<M>
> with N != M generate N+M totally independent
> instantiations of make_int_seq_impl.
>
>  From the source:
> "...but make_index_sequence is not hard to
> implement *(if efficiency is not a concern)*"
> [emphasis added]

Do you know of some good quality logarithmic implementation that I
can rip?

> [...]
>
> poly_collection.hpp:
>
> If poly_collection is defined in namespace detail,
> that means that your internal functions can be
> found by ADL for user defined types that mention
> poly_collection.  (I always avoid putting types
> that are part of the public interface in namespace
> detail to avoid this problem.)  Test case in test_adl2.cpp

I see. What's best?

a) moving poly_collection to boost::poly_collection::poly_collection_detail
b) moving poly_collection to
boost::poly_collection::detail::poly_collection_detail

> line 1109: "// TODO: can we do better than two passes?"
> This comment seems obsolete.

There's an implicit second pass in

https://github.com/joaquintides/poly_collection/blob/master/include/boost/poly_collection/detail/poly_collection.hpp#L1110

> [...]
>
> In Christ,
> Steven Watanabe

Thank you for your very valuable review! I'm bringing back home lots of
useful suggestions.

Best regards,

Joaquín M López Muñoz

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

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
On 23/05/2017 05:21, Joaquin M López Muñoz wrote:
>> "interspersedly"
>> This word sounds a bit awkward to me.
>
> Any less awkward synonim? For other readers' convenience, the statement in
> which the word appears is:
>
> "This mechanism is rendered mostly useless when derived1, derived2 and
> derived3 elements are traversed interspersedly without a definite pattern."

"interleaved", perhaps?

>> "to explicitly forbid"
>> split infinitive
>
> At the risk of starting a style war, it is my understanding that split
> infinitives are okay when used wth caution. Here, the alternative:
>
> "to forbid explicitly copying at"
>
> seems (to me) less clear as it might be parsed as referring to some sort of
> explicit copying...

FWIW, as a native English (or dialect thereof) speaker, the original
version sounds better to me, and not just because of the ambiguity of
the alternative.  As far as I am aware, objecting to split infinitives
is out of fashion, and was never all that popular to begin with.


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

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
AMDG

On 05/22/2017 11:21 AM, Joaquin M López Muñoz via Boost wrote:

> El 18/05/2017 a las 2:59, Steven Watanabe via Boost escribió:
>> I vote to ACCEPT PolyCollection into Boost.
>
> Thank you Steven! Sorry it took me so long to answer your post, got a busy
> weekend and yours is not a review one can deal with in a hurry.
>
>> Details:
>>
>> General:
>>
>> Missing .gitattributes
>
> Noted. By the way, I couldn't find this as a requirement in
> http://www.boost.org/development/requirements.html
>

The only reason I notice is that I can't open
any of the files in notepad...

>> [...]
>>
>> an_efficient_polymorphic_data_st.html:
>>
>> "interspersedly"
>> This word sounds a bit awkward to me.
>
> Any less awkward synonim? For other readers' convenience, the statement in
> which the word appears is:
>
> "This mechanism is rendered mostly useless when derived1, derived2 and
> derived3 elements are traversed interspersedly without a definite pattern."
>

  I think the sentence should be reordered.  The
problem, for me, is the use of -ly with a past
participle, which affects most direct synonyms as well.
"...useless for interspersed traversal of derived1..."
would be one possibility.

>
>> "Insertion mechanisms are rather..."
>> -> "The insertion mechanisms are rather..."
>
> Not being a native English speaker (nor Russian either, who typically have
> a hard time with definite articles :-) ), "Insertion mechanisms" looks
> perfectly
> OK to me, but I can change that anyway.
>

  Since you're talking about a specific set of
insertion mechanisms, I think it should have
an article.

>> "to explicitly forbid"
>> split infinitive
>
> At the risk of starting a style war, it is my understanding that split
> infinitives are
> okay when used wth caution. Here, the alternative:
>
> "to forbid explicitly copying at"
>
> seems (to me) less clear as it might be parsed as referring to some sort of
> explicit copying...

  It's not ambiguous.  "Explicitly" modifies "copying."
I.e., this isn't a correct rewrite of the sentence.
It would be either "explicitly to forbid copying"
(which is correct and unambiguous, but sounds pedantic)
or "to forbid copying explicitly"

> By the way, there are other instances of split
> infinitive in this
> section, for instance "to only address" (mentioned below).
>

  In this case "to address only" works fine.  Although
it changes "only" to modify "the elements" instead of
"to address", this doesn't affect the overall meaning.

>> "...it is possible to only address the elements of a given
>> segment ..."
>> Address here is a bit confusing as "address" has a specific
>> meaning in programming.  Access is probably a better term
>> to use.
>
> Will change that.
>
>> "cc.empty(index)...
>> Throws: If the indicated type is not registered. "
>> Does this really make sense?  I think it would be
>> more useful to treat non-registered segments as
>> empty.  Throwing here is somewhat inconsistent
>> with operator== which considers non-registered
>> segments to be equivalent to empty segments.
>
> Non-registered == empty would lead to stranger situations, IMHO: we
> can't hold the invariant that empty(index)==(begin(index)==end(index)) as
> begin/end(index) cannot return anything sensible for non-registered
> segments. Similarly, max_size, capacity etc. need to forward to the
> underlying segment to return something, unless we make up the return
> values for non-registered segments.
>

  The iterators are not dereferenceable, so they
don't actually need to point to anything.  capacity
is obviously 0.  max_size... yeah, I can see
that that would be a problem.

>> "cc.max_size()
>> REVIEW THIS DESIGN DECISION"
>> "the minimum return value of max_size for each of the segments of the
>> collection"
>> I think that the fundamental invariant of
>> max_size should be that size() <= max_size().
>> Both max_size and capacity are a bit strange.
>> My inclination would be to remove capacity
>> entirely (or rename it) and change max_size to
>> mean the maximum possible size of the whole container.
>
> This is a design area I'm definitely not happy with. Please follow this
> thread
> for a related discussion:
>
> https://lists.boost.org/Archives/boost/2016/11/231784.php
>
> I'm leaning towards dropping (global) max_size/capacity, as you and
> Thorsten
> suggest, till we find sensible semantics for them.
>

I don't think that there are sensible
semantics for capacity.

Expected semantics of capacity:
a) size() <= capacity()
b) if N <= (capacity() - size()) then N elements can
   be inserted without reallocation.
c) capacity() - size() is proportional to the amount
   of wasted memory.
d) reserve(N) has a post-condition that capacity() >= N
In particular, I think there's no way to satisfy
both (b) and (c) simultaneously.

On a related note, reserve(size() + N) probably has
unexpected behavior.

>> Class template base_collection
>> "subobject(x) = static_cast<Derived&>(x) with typeid(x)==typeid(Derived)"
>> static_cast excludes virtual inhertance, which I presume is unsupported.
>
> Ummm.. Hadn't thought about virtual inheritance. At first blush, seems
> like virtual
> inheritance would be automatically supported if only we fix subaddress as
> you suggest below, don't you think?
>

  I think so.  If there's anywhere else that you use
a downcast (perhaps stride_iterator.hpp:83), it
would also need dynamic_cast.  If this causes any
noticeable performance cost, I don't think it's
worthwhile.  Who uses virtual inheritance outside
of iostreams?

> (the "static_cast<Derived&>(x)" you refer to is only shown for
> explanatory purposes
> and does not appear anywhere in the actual code, as you probably guessed).
>
>> algorithm.hpp:
>>
>> 61:
>> segment_split: ADL (Contrived test case in test_adl1.cpp)
>
> Noted. Please enlighten me: is it OK to qualify the offending call as
> detail::segment_split, or should I fully qualify as
> ::boost::poly_collection::detail::segment_split?
>

  Any namespace is sufficient to suppress ADL.
Putting parentheses around the function name
also works.  (Personally, I always fully qualify
everything in headers, but that gets very tedious.)

>> Also it looks like this code is trying to allow an any
>> that doesn't use typeid_ to be stored in a segment
>> of anys (at least that's the impression that I get from
>> the definition of subobject(x) for any_collection in
>> reference.html).
>
> That's the intention. If I can't get the wrapped object I'll store the any
> objects at least.
>
>> Trying to do this will error out in
>> split_segment.hpp:303 in build_index when constructing
>> the value_type.
>
> I'm not seeing this. First of all, there's a bug in
>
> https://github.com/joaquintides/poly_collection/blob/master/include/boost/poly_collection/detail/any_model.hpp#L128
>
>
> s/any_cast<void*>/any_cast<const void*>. Once you do that, the following
> compiles fine:
>
> <snip>
>     using concept=boost::mpl::vector<
>       boost::type_erasure::copy_constructible<>,
>       boost::type_erasure::relaxed
>     >;
> <snip>
>

This compiles because relaxed implies typeid_.

>> callable_wrapper.hpp:
>>
>> [...]
>>
>> line 83:        return r(std::forward<Args>(args)...);
>> This fails to handle R = void correctly.  The return
>> value of the function should be ignored.
>> (Failing test case included: test_void_return.cpp)
>
> Got it. I guess I only need to
>
> s/return r(std::forward<Args>(args)...)/return
> static_cast<R>(r(std::forward<Args>(args)...))
>
> right?
>

  That makes it too permissive although
it's probably okay since you also construct
a std::function.

>> [...]
>>
>> integer_sequence.hpp:
>>
>> This can't possibly be the best implementation.
>> make_index_sequence<N> and make_index_sequence<M>
>> with N != M generate N+M totally independent
>> instantiations of make_int_seq_impl.
>>
>>  From the source:
>> "...but make_index_sequence is not hard to
>> implement *(if efficiency is not a concern)*"
>> [emphasis added]
>
> Do you know of some good quality logarithmic implementation that I
> can rip?
>

I typically use something like this:
template<bool IsBaseCase>
struct make_int_seq_impl;
template<>
struct make_int_seq_impl<false>
{
  template<class T, T N>
  using apply = typename next_integer_sequence<make_int_seq_impl<(N-1 ==
0)>::template apply<T, N-1>>::type;
};
template<>
struct make_int_seq_impl<true>
{
  template<class T, T N>
  using apply = integer_sequence<T>;
};

which is optimized for memoization rather than
for the cost of a single instantiation.

Actually, looking at the way you're using it,
it probably doesn't matter at all.

>> [...]
>>
>> poly_collection.hpp:
>>
>> If poly_collection is defined in namespace detail,
>> that means that your internal functions can be
>> found by ADL for user defined types that mention
>> poly_collection.  (I always avoid putting types
>> that are part of the public interface in namespace
>> detail to avoid this problem.)  Test case in test_adl2.cpp
>
> I see. What's best?
>
> a) moving poly_collection to boost::poly_collection::poly_collection_detail
> b) moving poly_collection to
> boost::poly_collection::detail::poly_collection_detail
>

I don't think it matters.  Either one should work.

In Christ,
Steven Watanabe


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

Re: AllocatorAwareContainers (was: Review of PolyCollection starts today (May 3rd))

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 15/05/2017 11:12, Glen Fernandes wrote:

> On Sun, May 14, 2017 at 7:02 PM, Gavin Lambert wrote:
>>
>> In particular, these rules seem to basically require that the node type be
>> trivially-constructible, because you can't legally call the node constructor
>> and then call the value constructor where the value is a field of the node;
>> you'd have to construct the node, destroy the value, then re-construct the
>> value, which seems well over the line into crazypants territory.
>>
>> It all seems very arbitrary to me.
>
> No. You could also design the node type to contain a
> std::aligned_storage_t<sizeof(value_type), alignof(value_type)>
> object (instead of containing a value_type object).
>
> That way after you construct the Node type with ::new(x)
> Node(args...); you an use std::allocator_traits<U>::construct(u,
> address, args...) with the address of that aligned storage.

Apologies for the delayed response, but I was distracted by other things.

If the node actually contains aligned_storage rather than T, doesn't
that then require reinterpret_casting everywhere (since everything will
expect a T*, not an aligned_storage_t<>*)?  Isn't that usually
considered a bad thing?

It also seems really error-prone, since
allocator_traits::construct/destroy accept pointers of all types, not
just their parameterised type, and then act based on the received type.

So given:

     struct node
     {
         node* next;
         std::aligned_storage_t<sizeof(T), alignof(T)> storage;
     };
     using alloc_traits = std::allocator_traits<T>;
     using node_traits = typename alloc_traits::
             template rebind_traits<node>;

     alloc_traits::allocator_type m_alloc;
     node_traits::allocator_type m_node_alloc; // m_node_alloc(m_alloc)

This code looks correct at a glance, but is horribly horribly wrong:

     node* n = node_traits::allocate(m_node_alloc, 1);
     ::new(n) node({ nullptr });  // not allowed to call construct?
     alloc_traits::construct(m_alloc, std::addressof(n->storage));
     // ...
     alloc_traits::destroy(m_alloc, std::addressof(n->storage));
     n->~node();       // not allowed to call destroy?
     node_traits::destroy(m_node_alloc, n, 1);

(The double-construction of storage isn't the wrong part -- we're
relying on it being a trivially-constructible type.)

This compiles, and it default-constructs and then destroys a T inside
node, right?  No, since storage is the wrong type it ends up calling the
storage-POD constructor and destructor, not T's.  Despite alloc_traits
supposedly being the traits for T.  Why is the allocator even templated
on this type when it doesn't actually use it?

To make that code actually behave as expected, you need to
reinterpret_cast<T&>(n->storage).  And you need to remember to do that
everywhere, and the compiler won't help you because it will happily
compile with the wrong behaviour if you forget.  (Only the allocator
parameter is type-checked, not the address parameter.)

Am I missing something?  This seems a bit broken.

Granted it's easy enough to make this safe once you're aware of it, but
it seems like a way-too-easy trap to fall into.  eg. this would be a
"safe" node:

     struct node
     {
         explicit node(node* n = nullptr) : next(n) {}

         node* next;
         T& value() { return reinterpret_cast<T&>(storage); }

     private:
         std::aligned_storage_t<sizeof(T), alignof(T)> storage;
     };

But of course now it's not a POD.


One other thing that the lax type-checking does allow is this:

     node* n = node_traits::allocate(m_node_alloc, 1);
     ::new(n) node();  // not allowed to call construct?
     node_traits::construct(m_node_alloc, std::addressof(n->value()));
     // ...
     node_traits::destroy(m_node_alloc, std::addressof(n->value()));
     n->~node();       // not allowed to call destroy?
     node_traits::destroy(m_node_alloc, n, 1);

(Specifically not actually using m_alloc or alloc_traits any more.)
Which seems like it should be illegal (despite being convenient, since
it means storing only one allocator), although a StackOverflow thread
claims that it's intended behaviour.


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

Re: AllocatorAwareContainers

Boost - Dev mailing list
El 15/06/2017 a las 10:17, Gavin Lambert via Boost escribió:

> [...]
>
> Granted it's easy enough to make this safe once you're aware of it,
> but it seems like a way-too-easy trap to fall into.  eg. this would be
> a "safe" node:
>
>     struct node
>     {
>         explicit node(node* n = nullptr) : next(n) {}
>
>         node* next;
>         T& value() { return reinterpret_cast<T&>(storage); }
>
>     private:
>         std::aligned_storage_t<sizeof(T), alignof(T)> storage;
>     };
>
> But of course now it's not a POD.

It's not a POD, but it has a trvial default ctor. My reading of 3.8
Object lifetime [basic.life]
implies that you can omit construction and begin using a just-allocated
object when
its default ctor is trivial. Maybe some language lawyer can shed some
light here.

Joaquín M López Muñoz

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

Re: AllocatorAwareContainers (was: Review of PolyCollection starts today (May 3rd))

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Thu, Jun 15, 2017 at 4:17 AM, Gavin Lambert via Boost wrote:
> If the node actually contains aligned_storage rather than T, doesn't that
> then require reinterpret_casting everywhere (since everything will expect a
> T*, not an aligned_storage_t<>*)?  Isn't that usually considered a bad
> thing?

Yes., and...

> It also seems really error-prone,

Yes. :)

i.e. It is easy to make the mistake you mentioned, among others.

The model isn't nice (but it is what we have, until someone designs a
less terrible one).

Glen

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