Re: cvs commit: squid3/src ftp.cc

Alex Rousskov wrote:
> On Mon, 2007-08-13 at 12:01 +0200, Guido Serassio wrote:
>
>> Now fixed.
>
> If you have time, it may be better to replace a virtual
> FtpStateData::haveControlChannel(char*) into a static
> FtpStateData::HaveControlChannel(FtpStateData*, char*) and let it check
> the ftpState pointer itself. This will avoid more code repetition and
> may even work faster.

Whats with the uppercasing? the method is not a type. Is this a new
convention not mentioned in the wiki/devel-faq?

The FTP code is all a it messy still with its partial conversion and
overlaps with other protocols code.
When the whole area is cleaned up several of those functions should be
inline. I was wondering about an *this check inside (does away with the
extraneous parameter), but I'm not certain enough to code it for 3.0.

>
> I would also suggest to use a different method name since the code
> assumes it is only called to detect attempts to call a method when a
> control channel is open. Also, doneWithServer() actually checks for the
> presence of both data and control channels.

Ah, quite right. doneWithServer() is only one of the cases that method
should handle.
I'll be adding the other soon.

Amos
Received on Tue Aug 14 2007 - 05:01:25 MDT