On Wed, 15 Mar 2006, David McCreedy wrote:

(still haven't reviewed the patch, mostly responding to the questions here)

>> - As someone pointed out here recently, the hooks to do EBCDIC conversion
>> could also be used to do UNICODE conversion; as such, most references to
>> EBCDIC could be changed to something more generic.
>
> Can someone verify that the hooks for EBCDIC conversion are actually
> suitable for this? If so, it's a great idea. I just need to know which
> parts apply.

We can at least assume or pretend that it will and can be used for other
translations than just EBCDIC <=> ASCII so a more generic name could be
appropriate I think.

Also, even if it wouldn't work 100% for other conversions it should still be a
pretty good starting point and then it could be fixed to work with others.

>> - The lack of proper ftp ASCII conversion is a KNOWN_BUG #21. Presumably,
>> this patch does the right thing for EBCDIC, but can it be made to do the
>> right thing in the general case?
>
> I think it can for gets. But my code doesn't address puts. If half is
> better than none, can someone look at the curl_convert_lineends code in my
> patched lib/sendf.c and tell me if it works on other platforms?
>
> And should this be optional? I can image some users would be startled if the
> line end handling changed. Even if it went from wrong to right.

For FTP ASCII transfers the current behavour is wrong and documented as wrong,
so any user who would be surprised by a correction are depending on something
they shouldn't. And besides, they could easily switch to binary mode and get
the old functionalty brought back.

>> - There are several places where #ifdefs could be removed from where they
>> are interspersed within the core curl code and changed to supply alternate
>> functions depending on the platform, e.g. curl_convert_lineends() and (a
>> suitably renamed) Curl_convert_ascii_to_ebcdic() could be no-ops or even
>> macroed away on non-EBCDIC platforms, making the code easier to read.
>
> That's the approach I've used with my Apache contributions, so I'm familiar
> with it. And I can certainly do that if it's preferred. Sometimes it's a
> toss up between looking at #ifdefs or tracking down a function to realize
> that it's been #ifdef'd away.

I'd say it is preferred. Tracking down a function only to find it #ifdef'd
away is more of a one-time operation, while the whole code flow is easier to
read when less #ifdef-ridden and that is something that's likely to be done
many times.

>> - curl_unescape2() shouldn't take a void *; that pointer must be a
>> struct SessionHandle *
>
> I made it a void pointer for backward compatibility and for use outside of
> libcurl.

Yes, it should actually more accurately use a 'CURL *' as that what other
external functions use for this. It struck me that perhaps a better name for
this new function would be curl_easy_[un]escape() to better match the other
functions that take a CURL * as first argument?

> Additionally, unless you need conversion there's no need for the
> SessionHandle in the escape functions.

Right, but that little implementaion detail won't be known to the outside
world.

-- 
  Commercial curl and libcurl Technical Support: http://haxx.se/curl.html