Re: [RFC] ICU UConverter implementation for ext/intl

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