Re: [boost] Formal Review: Assignment Library

From: Vladimir Prus (ghost_at_[hidden])
Date: 2004-04-08 04:26:05

Tom Brinkman wrote:
> The review of Assignment Library library, written by
> Thorsten Ottosen 2003 (nesotto_at_[hidden]) starts
> today(April 1, 2004)and runs for 10 days.

I think the library should be accepted, though I have some suggestions. The 
review below is based on reading the documentation for about an hour.

First off, I think such a library is usefull. Besides stl container 
initialization, it would be usefull for other libraries which want to provide
convenient initialization syntax.

Now, to the suggestions

1. The library expects the user to overload 'make_insertion'  for his types. 
Now suppose I have 'class Email' which list of 'to' and 'cc' addresses.I want 
to initialize both using the assign library, e.g

   Email e;
   e.add_to("Winnie-the-Pooh")("Ia");
   e.add_cc("Story writer");

The problem is that I can have only one 'make_insertion' for the 'Email' 
class. Exposing the real containers used for storing addresses is not good 
idea. And there might be 'add_to_address' function which does additional 
handling of argument, before pushing it into the container. So, it would be 
nice if the library allowed to specify a *function* which will do the actual 
insertion.

2.  The library examples contain

   using namespace assignment;

which is likely to burn users in a subtle way. Suppose:

   // some header 
   namespace email {
        class Email {};
        std::ostream& operator<<(std::ostream& os, const Email& e);	
   }

  // some source
  #include "some header"
  namespace email {

        using namespace assignment;

        void internal_function() {
                vector<int> v;
                ....
                v << 1, 2, 3;
        }
  }

The operator<< declared in the namespace will hide the one from "using 
namespace", and unless user is aware about this problem he might spend a lot 
of time trying to understand why the right operator<< is not selected. I know 
it costed me about 1 hour some time ago.

3. While the += operator is fine with me, I don't like the << operator. I 
cannot associate the conventional meaning of the operator with 'assign all' 
semantic, so I suggest that this operator is dropped completely. You can 
provide alternative syntax for assign_all:
  
     assign(v) = 1, 2, 3, 4, 5;

if 
    assignl(v)(1)(2)(3)(4)(5)

has too many paranthethis. (Since comma has lower predecence than =, the first 
syntax will really work).

4. The operator() and other operators in insert_assigner are declared inline. 
Suppose

   vector<some_type> v;
   assign(v)("first", "")("second", "");

This involves call to constructor of 'some_type' and possible calls to 
additional constructors needed to converted char* into types that 'some_type' 
excepts. If operator() is inline, those calls will be inlined as well, 
increasing code size. Similiar problem exists because single-argument 
operator() accepts a fixed type, possible requiring convertion on the 
caller's side.

I'd suggest the declare the operators outside of class declaration. This will 
give the compiler more freedom -- e.g. inlining only if maximum optimization 
level is requested. 

4. Just like other revievers, I'm not comfortable with monolitic stl.hpp

5. Operator += which is declared for all possible types makes me nervous.
IIRC, stl formatiing library by Terje Sletteb used some trick to avoid 
exposing all those operators. Something like

    template<template<class T, class Alloc> sequence>
    class helper { 
        friend std::ostream& operator<<(std::ostream&, const Sequence& s) { ... } 
   }
   // in vector.hpp
  template helper<std::vector>;

Terje said the explicit instantination bring the friend declaration into 
namespace. I'm somewhat rusty on friend lookup rules, so further 
investigation is needed.

Documentation issues

1. The example for maps uses:

   map<const char*, int> month;

which is a bit strange, even for example, given that comparison for "const 
char*" does not compare string values

2. In the graph example, I'm not sure how you add edge between vertices 4 and 
6, in a graph with only 5 vertices. If new vertices are automatically added, 
this should be documented. And BTW, what about graph which don't use ints for 
vertex_descriptor? 

3. I did not really understoon 'tuple_insert_assigner' role. Why is it needed?

- Volodya