Re: [PATCH] #2 Update on port to OS400/EBCDIC

Paul Burba <paulb@softlanding.com> writes:

> Philip Martin <philip@codematters.co.uk> wrote on 02/14/2006 12:24:08 PM:
>
>> Paul Burba <paulb@softlanding.com> writes:
>
>> > - ignore_content_type, APR_LOCALE_CHARSET,
>> > + ignore_content_type,
>> > +#ifndef AS400
>> > + APR_LOCALE_CHARSET,
>> > +#else
>> > + apr_psprintf (pool, "%i",
> APR_LOCALE_CHARSET),
>> > +#endif
>> > outfile, errfile, ctx, pool);
>> > }
>>
>> I'd much rather see the conditional stuff hidden behind some sort of
>> macro. Perhaps something like:
>>
>> #ifndef AS400
>> #define SVN_APR_LOCALE_CHARSET APR_LOCALE_CHARSET
>> #else
>> #define SVN_APR_LOCALE_CHARSET APR_STRINGIFY(APR_LOCALE_CHARSET)
>> #endif
>
> In V5R4 IBM defines APR_LOCALE_CHARSET this way:
>
> #define APR_LOCALE_CHARSET (int)1208

That's a bit odd, 1208 is already an int, I suspect it's the result of
someone blindly converting the normal APR macro.

> So we can't stringify it, we'd get "(int)1208". At least that's the way
> it works on OS400, I confess that the whole APR_STRINGIFY macro puzzles
> me:
>
> #define APR_STRINGIFY(n) APR_STRINGIFY_HELPER(n)
> /** Helper macro for APR_STRINGIFY */
> #define APR_STRINGIFY_HELPER(n) #n
>
> It doesn't look like it should do anything to me, if anyone wants to
> explain this they have my eternal gratitude, I have no cash prizes to
> offer :-)

Well, '#' is the standard preprocessor stringify operator that adds
double quotes around the argument to create a string literal. The
helper macro is a technique to ensure that macro argument expansion
occurs so that when APR_STRINGIFY is used on macros it operates on the
value rather than the name:

   #define foo bar
   APR_STRINGIFY(foo)

the result is "bar" rather than "foo".

> Anyway...
>
>> or (following apr)
>>
>> #ifndef AS400
>> #define SVN_APR_LOCALE_CHARSET APR_LOCALE_CHARSET
>> #else
>> #define SVN_APR_LOCALE_CHARSET ((const char*)APR_LOCALE_CHARSET)
>> #endif
>
> I took this second approach and put the following in svn_utf.h (as my
> other e-mail mentioned I'm open to putting this somewhere more
> appropriate, but for now it's here):

svn_utf.h is OK.

> #ifndef AS400
> #define SVN_APR_LOCALE_CHARSET APR_LOCALE_CHARSET
> #define SVN_APR_DEFAULT_CHARSET APR_DEFAULT_CHARSET
> #else
> /* APR_LOCALE_CHARSET and APR_DEFAULT_CHARSET are defined as ints on
> * OS400. */
> #define SVN_APR_LOCALE_CHARSET (const char*)APR_LOCALE_CHARSET
> #define SVN_APR_DEFAULT_CHARSET (const char*)APR_DEFAULT_CHARSET
> #endif
>
> The replacement for SVN_APR_DEFAULT_CHARSET isn't technically needed right
> now, but seems appropriate(?) to add to minimize confusion.
>
>> > --- subversion/libsvn_subr/utf.c (revision 18448)
>> > +++ subversion/libsvn_subr/utf.c (working copy)
>> > @@ -57,7 +57,11 @@
>> > destroyed. */
>> > svn_boolean_t valid;
>> > /* The name of a char encoding or APR_LOCALE_CHARSET. */
>> > +#ifndef AS400
>> > const char *frompage, *topage;
>> > +#else
>> > + int frompage, topage;
>> > +#endif
>> > struct xlate_handle_node_t *next;
>> > } xlate_handle_node_t;
>>
>> Is it possible to avoid changing xlate_handle_node_t and instead
>> convert the strings to integers just before calling apr_xlate_open?
>
> Yes, but even better, as I read your ideas I realized I don't need to use
> strings like "1208" anywhere. (const char*)1208 works fine and the only
> change needed is to cast topage and frompage to ints when calling
> apr_xlate_open().

Looks good to me.

>
>> > +#ifndef AS400
>> > SVN_ERR (get_xlate_handle_node (&node, topage, "UTF-8",
> convset_key,
>> > pool));
>> > +#else
>> > + /* On OS400 we assume topage is a string representation of a CCSID
> int
>> > + * and attempt to get a translation node for that CCSID. */
>> > + SVN_ERR (get_xlate_handle_node (&node, atoi (topage), 1208,
>> > convset_key,
>> > + pool));
>> > +#endif
>>
>> Rather than change all the "UTF-8" to 1208 can you recognise "UTF-8"
>> at runtime and convert to 1208 just before calling apr_xlate_open? If
>> you want to avoid the runtime conversion then I'd prefer a macro
>>
>> #ifndef OS400
>> #define SVN_APR_UTF8_CHARSET "UTF-8"
>> #else
>> #define SVN_APR_UTF8_CHARSET "1208"
>> #endif
>>
>> or
>>
>> #define SVN_APR_UTF8_CHARSET ((const char *)1208)
>
> I took the latter approach and put it in utf.c:

The only remaining issue is that some of the comments in the public
header files refer to APR_LOCALE_CHARSET. Perhaps they should change
to refer to SVN_APR_LOCALE_CHARSET? I'm not sure.

Thanks for reworking the patch, +1 to commit.

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org