FYI, these changes are committed. -Steve
> -----Original Message----- > From: Alan Conway [mailto:[EMAIL PROTECTED] > Sent: Friday, October 24, 2008 8:57 AM > To: [email protected] > Subject: RE: Question on change to C++ code in Url.h > > > 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. >
