Some hints about change in qpid/client/Connection.cpp (revision 693918)

[Manuel Teira Paz <mt...@tid.es>]

Hello.

Trying to get the solaris port in sync with trunk changes, I've found 
errors in a new overloading of qpid::client::Connection::open introduced 
in revision 693918 by aconway. It doesn't compile on solaris, and the 
guilty seems to be:

const TcpAddress* tcp = i->get<TcpAddress>();

The SunCC compiler doesn't like get as a method of Url, and as far as I 
see, it isn't. What I've made, after a fast read about boost:variant, 
was to change it to looks like this:

const qpid::TcpAddress tcp = boost::get<qpid::TcpAddress>(*i);

and using tcp later as an object, not a pointer.

I'm not sure about the qpid:: qualifier in TcpAddress being needed, 
since there's not a 'using namespace qpid' , are names from parent 
namespaces directly available in nested ones?

So, did it compile in gcc-linux in the previous form? And, do you agree 
with the changes made?


And a new question regarding this. Now that I should have commit rights, 
what's the way to proceed? For example, do you think a change like this 
one, should be discussed before commit? Should I open JIRA issues for 
these changes? Is there any document about commit policies in the 
project or something similar?


Best Regards.

Manuel.



PS: The diff of r693918 regarding qpid/client/Connection.cpp

Index: Connection.cpp
===================================================================
--- Connection.cpp      (revision 693917)
+++ Connection.cpp      (revision 693918)
@@ -23,6 +23,7 @@
 #include "Message.h"
 #include "SessionImpl.h"
 #include "SessionBase_0_10Access.h"
+#include "qpid/Url.h"
 #include "qpid/log/Logger.h"
 #include "qpid/log/Options.h"
 #include "qpid/log/Statement.h"
@@ -48,6 +49,37 @@
 Connection::~Connection(){ }
 
 void Connection::open(
+    const Url& url,
+    const std::string& uid, const std::string& pwd,
+    const std::string& vhost,
+    uint16_t maxFrameSize)
+{
+    if (url.empty())
+        throw Exception(QPID_MSG("Attempt to open URL with no 
addresses."));
+    Url::const_iterator i = url.begin();
+    do {
+        const TcpAddress* tcp = i->get<TcpAddress>();
+        i++;
+        if (tcp) {
+            try {
+                ConnectionSettings settings;
+                settings.host = tcp->host;
+                settings.port = tcp->port;
+                settings.username = uid;
+                settings.password = pwd;
+                settings.virtualhost = vhost;
+                settings.maxFrameSize = maxFrameSize;
+                open(settings);
+                break;
+            }
+            catch (const Exception& e) {
+                if (i == url.end()) throw;
+            }
+        }
+    } while (i != url.end());
+}
+
+void Connection::open(
     const std::string& host, int port,
     const std::string& uid, const std::string& pwd,
     const std::string& vhost,


PS2: Diff against trunk of my current version:

Index: Connection.cpp
===================================================================
--- Connection.cpp      (revision 698467)
+++ Connection.cpp      (working copy)
@@ -36,6 +36,7 @@
 #include <functional>
 #include <boost/format.hpp>
 #include <boost/bind.hpp>
+#include <boost/variant/get.hpp>
 
 using namespace qpid::framing;
 using namespace qpid::sys;
@@ -58,24 +59,23 @@
         throw Exception(QPID_MSG("Attempt to open URL with no 
addresses."));
     Url::const_iterator i = url.begin();
     do {
-        const TcpAddress* tcp = i->get<TcpAddress>();
+        const qpid::TcpAddress tcp = boost::get<qpid::TcpAddress>(*i);
+       
         i++;
-        if (tcp) {
-            try {
-                ConnectionSettings settings;
-                settings.host = tcp->host;
-                settings.port = tcp->port;
-                settings.username = uid;
-                settings.password = pwd;
-                settings.virtualhost = vhost;
-                settings.maxFrameSize = maxFrameSize;
-                open(settings);
-                break;
-            }
-            catch (const Exception& e) {
-                if (i == url.end()) throw;
-            }
+        try {
+            ConnectionSettings settings;
+            settings.host = tcp.host;
+            settings.port = tcp.port;
+            settings.username = uid;
+            settings.password = pwd;
+            settings.virtualhost = vhost;
+            settings.maxFrameSize = maxFrameSize;
+            open(settings);
+            break;
         }
+        catch (const Exception& e) {
+            if (i == url.end()) throw;
+        }
     } while (i != url.end());
 }
 

Re: Some hints about change in qpid/client/Connection.cpp (revision 693918)

[Gordon Sim <gs...@redhat.com>]

Manuel Teira Paz wrote:
> Hello.
> 
> Trying to get the solaris port in sync with trunk changes, I've found 
> errors in a new overloading of qpid::client::Connection::open introduced 
> in revision 693918 by aconway. It doesn't compile on solaris, and the 
> guilty seems to be:
> 
> const TcpAddress* tcp = i->get<TcpAddress>();
> 
> The SunCC compiler doesn't like get as a method of Url, and as far as I 
> see, it isn't. What I've made, after a fast read about boost:variant, 
> was to change it to looks like this:
> 
> const qpid::TcpAddress tcp = boost::get<qpid::TcpAddress>(*i);
> 
> and using tcp later as an object, not a pointer.
> 
> I'm not sure about the qpid:: qualifier in TcpAddress being needed, 
> since there's not a 'using namespace qpid' , are names from parent 
> namespaces directly available in nested ones?
> 
> So, did it compile in gcc-linux in the previous form? And, do you agree 
> with the changes made?

Alan, you're probably the best person to answer that question.

> And a new question regarding this. Now that I should have commit rights, 
> what's the way to proceed? For example, do you think a change like this 
> one, should be discussed before commit? Should I open JIRA issues for 
> these changes? Is there any document about commit policies in the 
> project or something similar?

We don't have any official policies on the c++ side of things at 
present, relying on common sense judgements by committers really.

My view is that changes for platform issues should probably at least be 
commented (in the code or at least in the svn log) so that people on 
other platforms start to learn what causes problems (and don't 
re-introduce the problem by later changes).

For some issues JIRAs may be extremely valuable. For others where the 
JIRA is created and closed as the fix is committed (and contains the 
same text as the svn log!), it seems unnecessary to me.

Its never a problem to discuss before a commit, its just a judgement 
call by individuals really. Everyone should also feel comfortable 
pointing out issues after they see a commit they have concerns over either.

And of course if people feel that a different way of working would be 
beneficial, they can bring that up for discussion also. Whatever works best.