Re: cvs commit: squid3/src ftp.cc

> On tis, 2007-08-14 at 23:01 +1200, Amos Jeffries wrote:
>> 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?
>
> Hmm.. it should be in the style guide somewhere.
>
> object local methods and variables start with a lowercase, global
> variables and static class members with an upper case.
>
>> The FTP code is all a it messy still with its partial conversion and
>> overlaps with other protocols code.
>
> Can you be specific on what you think need further conversion or
> overlaps?

Ah, sorry, I said that without taking a good overview look at it (yay
doxygen). At the line-level it seems to have a lot of global stuff. I must
have gotten FTP confused with the HttpRequest usages.

The main gist of my thinking already exists in the form of ServerStateData
for FTP at least.

Alex made some comments earlier about use of temporary buffers in several
of the protocol automata. And here is a short list of currently global
functions which I think should be provided either by the main body of
squid (non-FTP specific) or part of the classes private methods.

 is_month()
 escapeAC()
 ftpOpenListenSocket()
 ftpFail()
 ftpUrlWith2f() - should be part of URL class when its ready
 ftpTrySlashHack()

(maybe others, I haven't looked closely at each one yet)

maybe as ftpListParts open class:
  ftpListPartsFree()
  ftpListPartsParse()

>
>> Ah, quite right. doneWithServer() is only one of the cases that method
>> should handle.
>> I'll be adding the other soon.
>
> And quite likely the whole function is not needed any more after the
> state fixes, but it's a reasonable safeguard so I don't object it's
> addition.

truely.

Amos
Received on Tue Aug 14 2007 - 17:46:25 MDT