This is the mail archive of the cygwin-apps mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

RE: [Patch] Proxy command-line argument


On 31 January 2008 08:05, Vincent Privat wrote:

  Before we even get started:  Thanks for your contribution!  We're pretty
under resourced here and every little bit really helps  :)

> Is the patch OK ?

  It patched and compiles fine.  I haven't had time to test it yet, but it
looks fairly obvious so I expect it'll work fine.  Couple of questions:

+  std::string proxyString(((std::string)ProxyOption).c_str());

  I'm not a C++ expert, so maybe I'm missing something, but if I read that
right you're taking ProxyOption (type StringOption), calling the std::string
conversion operator to return a std::string, getting a const char* from that
and using it to build a std::string.  Why the trip via c_str?  Can't you just
construct the new std::string from the return of the conversion operator
directly by invoking the std::string copy c-tor?

+  std::string proxyString((std::string)ProxyOption);

  That seems simpler to me, but like I said I may be overlooking something.

+  if (proxyString.size())
+  {
+    NetIO::net_method = IDC_NET_PROXY;

  If parsing the string fails now, we'll be in an invalid state later won't
we?

+    unsigned int pos = proxyString.find_last_of(':');
+    if (pos >= 0)
+    {
+      NetIO::net_proxy_host = strdup(proxyString.substr(0, pos).c_str());
+      if (pos < proxyString.size()-1)
+      {
+        std::string portString = proxyString.substr(pos+1, 
                                          proxyString.size()-(pos+1));
+        std::istringstream iss(portString, std::istringstream::in);
+        iss >> NetIO::net_proxy_port;

  Do we really need to go to the lengths of building an istringstream here?
Wouldn't good old atoi be simpler? ;-)

+      }
+    }
+  }

  The only one of these that I think actually matters is the assignment to
NetIO::net_method, which I think we should move inside the "if (pos >= 0)";
the rest I'm just curious about.  I'll probably finish testing and commit it
at the weekend.


    cheers,
      DaveK
-- 
Can't think of a witty .sigline today....


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]