On Thu, 2008-10-23 at 16:47 -0400, Steve Huston wrote:
> Hi Alan,
>
> Thanks for the ideas... Here is some feedback.
>
> > On Wed, 2008-10-22 at 19:40 -0400, Steve Huston wrote:
> > > Hi Alan,
> > >
> > > I have a question on this change to qpid/Url.h:
> > >
> > > --- Url.h (revision 693917)
> > > +++ Url.h (revision 693918)
> > > @@ -45,7 +45,11 @@
> > > std::ostream& operator<<(std::ostream& os, const TcpAddress& a);
> > >
> > > /** Address is a variant of all address types, more coming
> > in future.
> > > */
> > > -typedef boost::variant<TcpAddress> Address;
> > > +struct Address : public boost::variant<TcpAddress> {
> > > + template <class T> Address(const T& t) :
> > > boost::variant<TcpAddress>(t) {}
> > > + template <class T> T* get() { return boost::get<T>(this); }
> > > + template <class T> const T* get() const { return
> > > boost::get<T>(this); }
> > > +};
> > >
> > > With this change, the Windows Visual C++ 8 compiler chokes.
> > Could you
> > > elaborate a bit on why the change is there and what may be the
> issue
> > > here? I've tried adjusting the template types to get around it but
> > > have had no success.
> >
> > The goal was to remove the requirement for explicit reference to the
> > boost namespace in user code
>
> In qpid users' code? Is this because Url is exposed in
> qpid/client/Connection?
Yes - you can open a connection with a URL. Going forward we will
probably want to move away from host/port and towards URLs as they
(will) provide a more flexible & uniform way of describing addresses in
multiple protocols.
> > and to make the Address class more self
> > contained. I find a get<T>() member is more intuitive than a get
> free
> > function.
>
> Right... I'm fairly new to boost variant, but the variant docs make
> the case that get() is pretty fragile itself and recommend using
> another way to access the variants. (apply_visitor).
Well that applies equally to a member get or a free function get. We can
add static visitor support later if we decide we need it. Using get()
makes you write
if (get<Foo>)
else if (get<Bar>)
else { ...nothing useful you can do here... }
This is fragile in the same way switch statements are: if a new type is
added to the variant you have to go update every if statement to handle
it. static_visitor is more robust *provided* you have a generic
templated way of dealing with arbitrary types that might be added to the
variant, otherwise it's similar to the switch:
struct my_visitor : public static_visitor {
void operator()(Foo&) {...}
void operator()(Bar&) {...}
template <class T> void operator()(T&) { .. generic default code .. }
}
Visitors are also much more convenient if you can handle all the values
you care about in a generic way, since then it's just a one-liner with
the templated op().
Adding a qpid::StaticVisitor to wrap up the boost one would be
straigthforward but since there's still only one address type for the
URL it's not urgent :)
>
> > Try the following, it gives better encapsulation for the boost
> stuff
> > and avoids the inheritance which is probably the cause of the
> > convert_construct confusion. I haven't tried to build this so
> > apologies
> > if I've missed something, I'm sure we can figure out
> > something portable
> > and now is the time if we need to change the API:
> >
> > struct Address {
> > public:
> > Address(const Address& a) : value(a.value) {}
> > explicit template <class T> Address(const T& t) : value(t) {}
> > template <class T> Address& operator=(const T& t) {
> > value=t; return
> > *this: }
> > template <class T> T* get() { return boost::get<T>(&value); }
> > template <class T> const T* get() const { return
> > boost::get<T>(&value); }
> >
> > private:
> > boost::variant<TcpAddress> value;
> > };
> >
> > Actuall adding an explicit copy constructor
> > Shout if this doesn't work.
>
> I had to make some adjustments, but the code below builds. I had to
> remove the "explicit" from the copy constructor else TcpAddress
> objects couldn't directly be assigned into a Url. Also the added
> insertion for Address objects (the implementation does get() call and
> then calls the TcpAddress insertion).
>
> Please let me know what you think of this...
>
> Thanks,
> -Steve
>
> struct Address {
> public:
> Address(const Address& a) : value(a.value) {}
> template <class T> Address(const T& t) : value(t) {}
> template <class T> Address& operator=(const T& t) { value=t;
> return *this; }
> template <class T> T* get() { return boost::get<T>(&value); }
> template <class T> const T* get() const { return
> boost::get<T>(&value); }
>
> private:
> boost::variant<TcpAddress> value;
> };
>
> std::ostream& operator<<(std::ostream& os, const Address& addr);
>
Looks good to me.
