httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <trawi...@bellsouth.net>
Subject Re: [PATCH] APR wrapper for iconv
Date Tue, 18 Apr 2000 16:37:52 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.

read the code I posted; we do have tables, but they are built using
iconv() instead of hard-coding them;

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

see the discussion on names further down

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

I don't buy the no-op argument.  It is not cool for an app to use APR
functions which translate from one codepage to another and for the
program not to be able to find out that nothing really  happened.
Actually, it isn't cool for an app to use any APR functions and have
them silently fail.

I'd prefer that if there is no real translation support then we do a
memcpy if the source codepage and the destination codepage are the
same but we fail the ap_codepage_open() otherwise.

If nobody else disagrees with you on this, I'm happy to oblige.

Is this the desired logic?  (I wondered what you meant since
you didn't use the autoconf-style preprocessor symbol.)

#ifndef HAVE_ICONV 

  spit out the no-op declarations/macros

#else

  spit out the real declarations

#endif

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

Does that mean "use lib/apr/iconv/foo/apr_iconv.c" where foo is the OS?

Somebody else can perhaps discuss code sharing further.  I just want
to know explicitly where you want the code.

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

I am not sure what you mean by this.  Is this justification for
duplicating iconv() usage among lib/apr/iconv/foo directories since
different platforms/RTLs may have things called iconv() that don't
work exactly the same way?

> >   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:
> 
> iconv_open()
> iconv_close()
> iconv()

Here is the pattern I see in the iconv() functions...

  X_open()
  X_close()
  X()

What about your names?  The only consistent pattern I see is "ap_".
                                         
ap_codepage_open()                       A_Y_open()
ap_translate_codepage()                  A_Z_Y()
ap_translate_char()                      A_Z_char()
ap_codepage_close()                      A_Y_close()

I think that ap_translate_buffer() instead of ap_translate_codepage()
is a little better, but there is still the discrepancy among the
different uses of codepage and translate.  Only one of these terms is
needed.

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

fixed...

> 
> > +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
> *outbuf,
> > +                                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) {
>     foo
> }
> else {
>    bar
> }

Before wasting time updating those no-op macros, I felt that some
discussion would be worthwhile.  Look at the text I wrote next to
those macros:

+    
+/* TODO: determine whether or not we always have these routines
+ * in APR and perhaps what to do if they aren't supported on
+ * some platforms (fail at compile time?  fail at link time?
+ * fail at run time?) */
+   
+#if defined(ICONV_IMPLEMENT_NYET)
 
-#if !defined(ICONV_IMPLEMENT)

I gather that your desire is that they fail at runtime but that the
application shouldn't know that they failed.  I didn't even realize
that this was a possibility.

-- 
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
     http://www.geocities.com/SiliconValley/Park/9289/
          Born in Roswell... married an alien...

Mime
View raw message