Thanks for the quick feedback.
Below are my comments... and a few questions.

On Tuesday, 14 Mar 2006, Dan Fandrich wrote:
>
>
>I have a few general comments:
>
>- 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.

>- Is it really necessary to provide callbacks for a user-supplied
>conversion
>routine? Can't you rely on libiconv to do the right thing and let the user
>create a custom iconv conversion table if necessary?

Yes. Some TPF users do not have iconv.
Others may use a different conversion method for performance reasons.

>- 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.

>- 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.

>- 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.
The documentation on curl_unescape and curl_escape note that you need to
include curl/curl.h.
But the SessionHandle is defined in lib/urldata.h.
If I change it to a SessionHandle pointer it will break any code that isn't
already including lib/urldata.h.
Additionally, unless you need conversion there's no need for the
SessionHandle in the escape functions.

-David McCreedy
Received on 2006-03-15