Am Freitag, 29. Dezember 2006 19:54 schrieb Abdelrazak Younes: > Georg Baum wrote: > > You will probably get some from Lars, but I have one of my own: I don't > > like the replacement of iconv_t by int and the resulting casting. Please > > revert that. The casting of -1 to iconv_t is safer. Do you really know > > that int will always be compatible to iconv_t? > > I am not changing the semantics at all. The casting happens in any case, > be it internally or externally to the iconv_convert function. I just > factorized the number of casting, that's all. The result is identical to > the old code.
No. You changed some variables of type iconv_t to type int. That might result in identical code on your platform, but nobody knows whether that is the same on all other platforms. What I mean is that it is safer to cast -1 to iconv_t, and that all variables that store conversion descriptors should be of type iconv_t (as it was before). And if you don't like the many casts of of -1 you could add a const iconv_t invalid_cd = (iconv_t)-1; and use it. Then you have only one cast. > Besides, this static variable is declared all other the place is not > used anywhere except within iconv_convert so I really don't see why we > keep it. Which static variable do you mean? > > IMO an important bit is missing: Completing a conversion that is incomplete > > because the buffer was too small. > > Read more closely the code, I don't need a fixed size buffer any more. I > am writing the converted data right into the docstring output that has > already enough place. Sorry, I was unclear. That has nothing to with your changes but was a general comment (and I did not mean that _you_ should fix that problem): The static buffer ist still there in the other iconv_convert. > > We cannot always increase the bufffer if > > that happens, so IMO we need to do that for 1.5.0, because users will > > otherwise get silent data corruption. > > As said above, we don't need to do that anymore with my version. And why is it still there then? > > With that in mind I do not like that you make iconv_convert a part of the > > public interface. It has an ugly interface that should be kept private. > > Come one, it is only used in docstring. I can remove it from the > unicode.h and declare it as extern in docstring.C if you prefer. That would be worse IMO. > > It > > should be possible to do an efficient but still clean design IMHO. > > Be my guest then. Not enough time... Georg
