You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Steve Huston <sh...@riverace.com> on 2008/10/23 01:40:38 UTC
Question on change to C++ code in Url.h
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.
Compiling...
Url.cpp
C:\Program Files
(x86)\boost\boost_1_35_0\boost/variant/variant.hpp(1342) : error
C2666: 'boost::variant::convert_construct' : 2 overloads have similar
conversions
with
[
T0_=qpid::TcpAddress
]
C:\Program Files
(x86)\boost\boost_1_35_0\boost/variant/variant.hpp(1327): could be
'void boost::variant::convert_construct(const boost::variant &,long)'
with
[
T0_=qpid::TcpAddress
]
C:\Program Files
(x86)\boost\boost_1_35_0\boost/variant/variant.hpp(1258): or 'void
boost::variant::convert_construct(T &,int,boost::mpl::false_)'
with
[
T0_=qpid::TcpAddress,
T=qpid::Address
]
while trying to match the argument list '(const qpid::Address,
long)'
C:\Program Files (x86)\Microsoft Visual Studio
8\VC\include\xstring(634) : see reference to function template
instantiation 'boost::variant::variant(const T &)' being compiled
with
[
T0_=qpid::TcpAddress,
T=qpid::Address
]
RE: Question on change to C++ code in Url.h
Posted by Steve Huston <sh...@riverace.com>.
FYI, these changes are committed.
-Steve
> -----Original Message-----
> From: Alan Conway [mailto:aconway@redhat.com]
> Sent: Friday, October 24, 2008 8:57 AM
> To: qpid-dev@incubator.apache.org
> 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.
>
RE: Question on change to C++ code in Url.h
Posted by Alan Conway <ac...@redhat.com>.
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.
RE: Question on change to C++ code in Url.h
Posted by Steve Huston <sh...@riverace.com>.
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?
> 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).
> 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);
Re: Question on change to C++ code in Url.h
Posted by Alan Conway <ac...@redhat.com>.
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 and to make the Address class more self
contained. I find a get<T>() member is more intuitive than a get free
function.
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.
Cheers,
Alan.