httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
Subject Re: [PATCH] APR wrapper for iconv
Date Tue, 18 Apr 2000 12:44:49 GMT

> Currently, ap_translate_buffer() is approx. 11% slower on OS/390 
> than 1.3's ebcdic2ascii().

I would think the reason for that is because in 1.3, OS/390 was using
tables, and that hasn't been implemented in APR yet.  It can be
implemented, it just hasn't been yet.

> Changes from Ryan's sketch of apr_iconv.h, beyond function 
> signatures:
> 1) ap_translate_codepage() is renamed to ap_translate_buffer() for
>    no good reason
> 2) no special preprocessor symbol is required to build this into
>    APR; if iconv() isn't available, nothing will be supported
>    since there is no fall-back mechanism; ap_codepage_open() will 
>    fail at run-time;

The first is a name change for no good reason, and I dislike that because
the macros have been there for at least three months, even if nobody is
currently using them.  I can live with that though.  I can't live with the
second change.  MOST Unix platforms will want a no-op for the ICONV
stuff.  That is just a fact of life currently.  That preprocessor symbol
is what provides us with the information to determine whether iconv should
be a no-op or not.

> Should the file be apr/lib/apr_iconv.c or apr/misc/unix/apr_iconv.c? 

Neither.  Why are we overloading things?  The lib directory is
specifically meant for code that was moved from 1.3, and really shouldn't
have anything added to it.  The misc directory is meant for things that
don't fit anyplace else.  I am just as guilty as anybody of overloading
misc.  Iconv implementations should have their own directory.  

At some point, if iconv is really needed, I would expect Windows to add it
to their code (if it isn't already there), and they are likely to really
break things when they do.  That is not a crack against MS, it is a fact
of life.

>   ap_codepage_open()
>   ap_translate_buffer()
>   ap_translate_char()
>   ap_codepage_close()
> and the "handle" is ap_iconv_t.
> I prefer to change these names at some point (either to be more 
> consistent with iconv() or just more consistent among themselves), but 
> there is no need to do that immediately unless somebody feels like
> thinking about it now.

What do you mean more consistent with iconv() or more consistent among
themselves?  Please specify where these aren't consistent with iconv.  I
have just checked the Single Unix specification, and iconv has:


This looks awful similar to what we have IMO.

> +
> +typedef struct ap_iconv_t ap_iconv_t;
> -typedef struct ap_iconv_t            ap_iconv_t;

Please use white space in typedefs, it makes the code so much easier to
read later.

> +ap_status_t ap_codepage_open(ap_iconv_t **convset, const char *topage, 
> +                             const char *frompage, ap_pool_t *pool);
> +    
> +ap_status_t ap_translate_buffer(ap_iconv_t *convset, const char *inbuf, 
> +                                ap_size_t *inbytes_left, char
> +                                ap_size_t *outbytes_left);

Until your macros for platforms that don't have iconv (or just aren't
compiling the code) evaluate to an ap_status_t, you can't change these to
return an ap_status_t.  This is why they were void to begin with.  For
example, consider the following bit of code:

if ((ap_translate_buffer(foo, bar, bax, "foobar", out) == APR_SUCCESS) {
else {

On platforms that do not want to compile iconv support, this will fail
miserably, because the no-op macro is not evaluating to APR_SUCCESS, so it
is executing the wrong code path.

>   * at a time.  This needs to be written carefully so that it works
>   * with double-byte character sets. 
>   */
>  void ap_translate_char(ap_iconv_t *convset, char inchar, char outchar);
> -void ap_codepage_close(ap_iconv_t *convset)
> +
> +ap_status_t ap_codepage_close(ap_iconv_t *convset);
> +

Same as above.


Ryan Bloom               
406 29th St.
San Francisco, CA 94131

View raw message