Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:63717
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: softfail (pb1.pair.com: domain golemon.com does not designate 209.85.223.170 as permitted sender)
MIME-Version: 1.0
Sender: php@golemon.com
In-Reply-To: <CAEZPtU6JQKSvh9b=QMmYfvPJWP-ZdMBOX9bd8J9=ytHu1gz_=Q@mail.gmail.com>
References: <CAESVnVrJZ0-FQPMdZUD+5QE2nrZGZFqXpcVP4ftkZQXewY1iHQ@mail.gmail.com>
	<CAEZPtU6JQKSvh9b=QMmYfvPJWP-ZdMBOX9bd8J9=ytHu1gz_=Q@mail.gmail.com>
Date: Tue, 30 Oct 2012 09:11:11 -0700
Message-ID: <CAESVnVp0zG=R6WHoeUNSjAcBwksWWpzQXWxBmq3GNZ_2Sq5wjw@mail.gmail.com>
To: Pierre Joye <pierre.php@gmail.com>
Cc: PHP internals <internals@lists.php.net>
Content-Type: text/plain; charset=ISO-8859-1
Subject: Re: [PHP-DEV] [RFC] ICU UConverter implementation for ext/intl
From: pollita@php.net (Sara Golemon)

> - the UCNV prefix is not necessary, we are in the UConverter class
> already, I would simply use LATIN_1, or ILLEGAL for exampe.
> - I would add ERROR_ to the error related constants (UNASSIGNED & co), or?
>
For the UConverterType enum I could maybe get behind that.  For
UConverterCallbackReason I think we need something, but ERROR_ isn't
universally applicable since three are just status incidents
(UCNV_RESET, UCNV_CLOSE, UCNV_CLONE).  Perhaps: REASON_* for these?
In that vein, I'd be tempted to make the UConverterType values be
TYPE_*

> - It is OO, we should or could use exception. Warning are not nice to
> deal with and errors are quite common during conversion
>
We are.  UConverterException is thrown everywhere but one place.  That
one exception is the static method I added to give a non-oop-like API
for the most core functionality.  Given its non-oopness, I left it
non-exceptiony.  See Gustvo's notes about error handling below.

> Some questions, is 'latin1' the same than const UCNV_LATIN_1? or is
> there any relation between the two?
>
The UCNV_LATIN1 constant is a numeric value, so they're not the same,
no.  The relationship is that when you instantiate a UConverter with
'latin1' (or 'iso-8859-1', or any of its aliases),
$conv->getSourceType() (or getDestinationType() as appropriate) will
return UConverter::UCNV_LATIN_1.

Each type in the UConverterType set uses a different algorithm
approach to do the character conversion.  Sets like latin1, utf*,
ascii, etc... can be done faster than sets like 'koi8-r' because the
conversions are simple bitshifts (very simple in the case of
ascii/latin1).  This just surfaces some info on what the converter is
doing internally and isn't much use beyond a bit of logging, in
practice.

> btw, can you add the changes to config.w32 too pls?
>
Ah, right sorry, had meant to do that at the last minute.  Will add
that in a bit,

> Error handling is done in a different way from the rest of the extension.
> Unusual as this may be, it would be a bad idea to introduce inconsistency
> here. See the implementations in the other modules.
>
I do agree, I was hoping I could slip that one by. :p  I'll take
another look at the rest of ext/intl and see about adapting my error
handling.

> The tests have poor coverage.
>
The important bits are hit, but yeah, it could stand to have better
testing.  I'll add some.