Quoting Gennadiy Rozental: > Here is my a bit late review for the variant library. In spite of > several > concerns that I have, I incline to vote to ACCEPT this submission.
Hi Gennadiy, thanks for the comments. I apologize for my late response. [snip] > Design > _________________________________________________ > > In most part design of the library looks solid and well thought-out( > I think we definitely ought to give Andrei credits for this also). As has been discussed since your posting, Andrei will receive credit for his OOPSLA 2001 paper and his C/C++ User Journal insofar as those works inspired the design of Boost.Variant. > There are several things that bother me though. > > 1. Two types requirement > It's unreasonable. ! or even zero sized variants should be allowed. I know you had not followed the review discussion closely, but we decided similarly as you did. There was once justification for this design decision, but it no longer applies. Accordingly, the requirement will be removed, with variant<> simply as shorthand for variant<void>. > 2. Top level const requirement on bounded types. > It's unreasonable. I should be able to define variant with const > types. It will be as usable as usual constant types are. The only > requirements that we may incur is that if one types is const, > rest should be also. It's actually not unreasonable: one of the primary goals of variant is to match the semantics of its bounded types as closely as possible. The prohibition on top-level const types, then, is quite reasonable. To see, consider the following: const int i = 3; i = 4; // error variant<const int> v; v = 4; // error? If top-level const types *were* allowed, then the assignment would succeed. Itay and I decided such was highly undesirable. Let me know if you disagree with the above reasoning. > 3. Copy Constructible/Assignable requirements on bounded types > This only need to be required if variant should have appropriate > feature. I disagree. As-is, every variant object requires CopyConstructible bounded types, as it is the only way to construct its content. Some notes, however. I may be able to eliminate the Assignable requirement altogether by modifying the implementation of variant::swap. As well, there has been some discussion about in-place construction, which could eliminate the CopyConstructible requirement except in cases of actual variant copying. (This conversation occurred with regard to optional, but could work with equal applicability to variant.) > 4. DefaultConstructible requirements on first bounded types > This only need to be required if variant need to be default > constructible. Agreed. > 5. Usage std::type_info for reflection > I don't think we should enforce RTTI for the variant users. We should > be able to postpone the decision on what kind of reflection > information user want till instantiation time. Please elaborate on this point. FYI, the current variant::type method is provided so as to mirror boost::any. > 6. extract > I not like this name. It does not reflect the essence of the > operation it performs. It does not extract juice from orange. It > provides an access to the varant value. It basically external > access method. So the name get, get_value would be more > appropriate. This issue was extensively discussed during the review, but I am not sure it came to any definite resolution. I am currently looking into the proposal by Joel de Guzman's to provide a 'get' function such as used by the tuple library. > Also I think we need free function form of value > extraction. In other case it would be difficult to place extract > in context where template parameter is deduced. And check function > is not that important in most cases. While I am again considering a free function, I'm not sure what difference it makes. Please elaborate. Also, I think the functionality offered in extract::check is quite important. Unlike visitation, extract (or get, or whatever) handles only one of several possible states of the given variant object. > 7. Variant size > Unfortunately I was not able to follow exact logic behind usage of 2 > different storages. But one thing I know: > sizeof(boost::variant<int,std::string>) could not be 28. > >From what I view it seems that types that are used to construct > storage2 also used when constructed storage1. So we definitely have > size duplication here. The two storages implement Anthony William's "double storage" technique. (See http://aspn.activestate.com/ASPN/Mail/Message/boost/1314807 for an overview.) This technique is necessary to provide a general guarantee of strong exception-safety, which in turn is necessary to maintain a "never empty" invariant for variant. In regard to the particularly large size you report, I believe it results from a problem either with boost::type_with_alignment itself or with my understanding of it. Thus, I am aware of the problem, but I am still determining how best to address it. > Separate issue is the type of which field. Having it as int is an > overkill. It would be enough in majority of the cases have char. But > we should be able to deduce the correct type depends on number of > argument types. You're probably right: I doubt more than 127 types will ever be needed. Still, this is an implementation issue, and variant::which() will return int. > 8. Visitation algorithm > In sketch presented visitation algorithm look like this: > > void foo1( which, visitor ) > { > if( n = 1 ) > visitor(...) > else > foo2( which, visitor ); > } > > void foo2( which, visitor ) > { > if( n = 2 ) > visitor(...) > else > foo3( which, visitor ); > } > > .... > > In a pseudo code it could be rewritten like this > > foo( visitor ) > { > if( which == 1 ) visitor( first field ); > else if( which == 2 ) visitor( second field ); > > ... > else if( which == n ) visitor( nth field ); > } > > It's obvious that switch-based algorithm will be more effective. And > I believe that given at compile time number of the types supplied > (or maximum possible types variant accepts we should be able to > generate one (even if we will need to use PP for that ) I'm not sure it's obvious, or even true. These functions are inlined, and as of yet I have no reason to doubt my code will "unroll" to match yours. Admittedly though, I have not inspected any disassembled compiler outputs. Let me know any results you may uncover. > 9. Design of the container move function > I do not see a way how with current design and implementation of the > container move function I could move content of one vector into > another originally empty vector. > move(src.begin(),src.end(),trg.begin() ) will move to garbage memory > move(src.begin(),src.end(), back_inserter( trg ) ) will copy instead > of move Do we need something like back_move_inserter? Perhaps. Nonetheless, Boost.Move is not a public library but only an implementation detail of the Boost.Variant library. I will consider this issue, however, in preparing Boost.Move for formal review. Thanks. > Implementation > _________________________________________________ > General comment: I believe that all template functions in header > files need to be defined inline. Noted, thanks. > static_visitor.hpp > Line template <typename R = void> fails to compile with MSVC6.5. SO > to make it work I was forced to use following form > template <typename BOOST_MPL_AUX_VOID_SPEC_PARAM(R)> > This and also the fact that MSVC6.5 could not handle return void > construct, forced me to change all the visitors defined in visitor.hpp to > return boost::mpl::void_. Maybe you could find better workaround I will look into this. Are there other MSVC6 compatibility problems you've noted? > static_visitable.hpp > 1. I believe that implementation of this concept could be enhanced. > The major problem as I see it is that static_visitable_traits could not > be instantiated with const type. That force you to propagate switch on > const/non const in many different places. While you could do this > only once in static_visitable_traits implementation. See attached file > for details. Once you do this the implementation of unary > apply_visitable will became as simple as this: > > template <typename Visitor, typename Visitable> > typename Visitor::result_type > apply_visitor( Visitor& visitor, Visitable& visitable ) > { > return static_visitable_traits<Visitable> > ::apply_visitor(visitor, visitable); > } > > You will see simplification in several other places. I'll look into this. Thanks. > 2. is_static_visitable description states that this metafunction will > check whether the specified type is visitable by a static visitor. > This statement is misleading, cause you do not (and could not ) check > this. What you check is that type could be visited using > static_visitable_traits interface. I never mention static_visitor > here. The only way visitation by a static visitor can happen is if the specified type derives static_visitable directly OR if it appropriately specializes static_visitable_traits. Accordingly, what is_static_visitable determines is whether objects of the specified type derives static_visitable OR if static_visitable_traits is specialized. Please elaborate if you feel something is wrong in my approach. > extractable.hpp > 1. First of all this is bad name. extractable implements different > concepts that it names. We want name type T extractable to announce > that this type support extracting something out of it. Isn't name > is misleading and means namely opposite that type is extractable > from something? Noted, thanks. (The juice is extractable, not the orange itself.) As noted above, however, the overall naming issue regarding "extractable"/"get"/etc. is yet to be resolved. If you have any suggestions, I'd welcome them. > 2. Extractible is the concept and should contain is_extractable > check, which is for some reason located in details of extract > header and named is_directly_extractable (this name even > contradicts the comment in it's definition: it's either > directly extractable or through custom traits implementation) If you looked more closely, you would notice that the is_directly_extractable is in a detail namespace. That said, I'm not sure what you mean when you say the the name "contradicts" its comment. Please explain. > 3. This header suffer from the same issue as static_visitable with > missing const/non-const switch. For example, once I implemented it > I was able to eliminate one layer in extract implantation in > extract.hpp. See attached file for details As I wrote above, I will look into this issue. Thanks. > extract.hpp > 1. boost/addressof.hpp include is missing Noted, thanks. > 2. From what I view in this header there are 3 ways to make type T > work with extract<T> interface > a) Make T "directly extractible", iow inherit from extractable > b) Make T "indirectly extractable" by means of specializing > extractable_trats > c) Make T visitable by extractor_visitor, by means of specializing > static_visitable_traits for T Regarding a) and b), you are correct. And while your comment in c) is technically, your wording suggests to me you might not understand visitation completely. The type T you describe does not support the given visitor explicitly. Rather it takes visitors via templatized parameters. > BTW it may worth mentioning in docs. Now it appears that having > extract::visitor in private part of the class definition make third > way less usable cause I could not mention Visitor name I'm not sure what you mean here. The visitor is _not_ intended to be used directly. > 3. Defining result type in extract::visitor definition is unnecessary > and in fact prohibit compilation on MSVC6.5 Please explain. I have not attempted compilation on MSVC prior to version 7. [snip] > 5. Name for the visitor could be more specific. For example the one > you have in it's description I don't understand why it needs to be more specific. It is a private nested class. If you disagree, please explain why. > define_forwarding_func.hpp > This file contain mix of Unix/Windoos EOLs. Please explain this problem and tell me how to fix it. Thanks. > mpl/contain.hpp > I think this is in Alexey direction: it may worth generalizing this > function so that if the second argument is the sequence it will > check that one sequence contain another. This was included only because mpl::contains as provided in Boost 1.29 contained a bug. This bug has been fixed in the 1.30 release. > mpl/guarded_size.hpp > I do not understand purpose of this header. Why couldn't we use >,< > signs instead? The purpose of guarded_size is to determine the size of a MPL sequence without counting the entire length. I implemented this code as an optimization since the only necessary information was whether the sequence given to variant was at least two elements. Note, however, that since I plan to modify variant to allow zero or one elements, guarded_size is no longer needed for variant. I don't know if it might find general utility elsewhere. > unary_apply_visitor.hpp > As I mention in static_visitable discussion, once you fix the traits > implementation it simplifies dramatically. I did not touch binary one > it may also became more simple. BTW what is the meaning of the NOTE > in this fie? As I wrote above, I will look into the matter of the traits template. However, the great bulk of the complexity in the file relates to working around the forwarding problem (i.e. proper handling in the face of const and non-const lvalues and rvalues). Regarding your question about the "NOTE": the workaround I present is only necessary on certain broken compilers, but it doesn't prevent functioning on conformant compilers. That is the meaning of the note. > delayed_apply_visitor.hpp > I did not touch binary side but once you fix the traits > implementation unary function operators became much more simple. > You don't even need PP anymore, but straightforward: > template <typename Visitable> > result_type operator()( Visitable& visitable ) > { > return apply_visitor(visitor_, visitable); > } Again, the code you provide does not allow temporaries to be passed to operator(). > has_trivial_move.hpp > From what I view it seems that: > has_trivial_move_constructor <= has_trivial_copy > has_trivial_move_assign <= has_trivial_assign > So your formulas could be simplified. My guess is that users would most commonly specialize has_trivial_move. For this reason, I make has_trivial_move_constructor and has_trivial_move_assign dependent on has_trivial_move rather than the other way around (as you suggest). Please let me know if you disagree with my reasoning. > incomplete.hpp > This header presents yet another implementation of smart pointer > idiom. In this case with deep copy semantic. Do we need one? If > yes, lets name it deep_copy_ptr. boost::incomplete is _not_ a smart pointer. It is a value type thats allocates its content on the heap. So no, deep_copy_ptr is not an appropriate name. > aligned_storage.hpp > 1. max_align used by implementation is not defined for Borland I was not aware of this since I never tested on Borland. Apparently I will need to discuss with John Maddock to find a workaround. (I will need to do this anyway since, as I noted above, either there is a problem with type_with_alignment or with my understanding of it.) > 2. aligned_storage component is not that generic as it may look from > the perfunctory look. It only works of size is 2^N. May be we should > check this explicitly for more clear error notification. Please explain. > 3. Why is it noncopyable?? The intention is to prevent blind copying of aligned_storage since it is not possible to invoke the copy constructor corresponding to the storage's content (if any). So I ask, why *not* noncopyable? > visitor_ptr.hpp > 1. why visitor_t is not function1<>? I'm not well accustomed to the Boost.Function library, but my limited understanding suggests the "catch-all" behavior of visitor_ptr_t is not implementable. Further, I am working on a larger Boost.Visitor library. That is, visitor_ptr will need to return a function object that serves as both a static and dynamic visitor. (See the code in the sandbox for further explanation.) > 2. Why constructor expect reference not the object? No particularly good reason. I suppose I will change this. > 3. Couldn't we use std::ptr_fun or it's boost relatives somehow? No. See above. > move.hpp movable.hpp > I think this functionality deserve better place in boost hierarchy > than variant details. I agree. However, it was not my intention for Boost.Move to undergo formal review at this time, and it would not have been approprate to "sneak" it in with Boost.Variant. I would like to emphasize, then, that your comments on my move-related code, while helpful to my future endeavors, are not particularly important to the Boost.Variant library itself. Therefore, I will ignore them until a later point. FYI though, I am working to ready Boost.Move for review and plan to submit my code shortly. I will take your feedback into consideration. Thanks. [snip] > variant.cpp > 1. You should have relied in latest cvs for review 1.29 workarounds > only confusing things I disagree: the CVS by its nature is not stable. I did include some code to support experimental variant<TypeSequence> syntax, and I apologize for any confusion this may have introduced. Of course, the issue is now moot. > 2. max_value metafunction description need to be stated more clear. > Is calculate maximum value of the function on a sequence, isn't it? Yes. Sorry for any ambiguity. > 3. What is purpose of variant_base? What is the difference between > detail::variant_base<Variant> and static_visitable<Variant>? This is a holdover from the code as implemented in the sandbox, where variant derives other classes such as moveable. In that case, variant_base eliminates the need to multiply use the PP to enumerate the template parameters for variant. > 4. using_storage1 implementation could use compile time if instead > of runtime one I suppose. On the other hand, I imagine any decent optimizing compiler would eliminate the if branch to the same effect. I'll look into it though. > 5. variant class contain a lot of things in private section Why did > not you separate them out of class as you did with visitors? It seems to be > possible. Yes, I it would be possible. However, it would require more use of preprocessor to templatize these helper classes appropriately. > 6. In general it may worth to separate this file in smaller parts. It > would help when you will work on portability issues. Noted. I'll look into it. > 7. Unlike rest of the submission I was not able to make this header > work on MSVC6.5. It will require more time and attention from authors. The docs clearly state the submission had been tested only on MSVC7 and GCC3. Nonetheless, I do plan on working for compatibility with Metrowerks, BCC, MSVC6.5 (if possible), and others as I have time. > Docs > _________________________________________________ [snip] > Tests > _________________________________________________ Your points on docs and tests are noted. I'll talk to Itay about this. We had split the work, where he would write docs and tests and I would handle implementation. [snip] > 4. Why variant_test is separate, in wrong place and is not in > Jamfile? This is the test file I had been working with before Itay developed the code in variant/test. I included the code in the review submission to demonstrate additional use of variant. I do not plan to include variant_test.cpp in the release. > Code > _________________________________________________ > > In most part code looks very clean and consistent. Only several > remarks: > > * I would prefer if you would follow boost recommendation with class > data member naming with leading m_ instead of trailing _. Where are these recommendations? > * extra indent of function result type look strange from my POV. Not to me. Others? > * private section of class definition with class representation > (data members) I would put on the bottom of the class OK, I suppose I could do that. > * do we need that complex hierarchy in variant detail subdirectory? This is due to the transition from the sandbox to the review submission. I'll be cleaning this up before release. > * several files have incorrect/missing file names in the comment on > top of the file (especially wrong path) I'll check it out. Thanks. > _________________________________________________ > > My regards to the authors for this interesting and complex work. > > Gennadiy. I appreciate all your comments and your vote for acceptance. Thanks, Eric _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
