Re: LibIDN code quality suggestions

[Simon Josefsson]

Bittner Ede <address@hidden> writes:

> Hello Simon,
>
>       Thank you for your fast reply!
>       It's nice to have the fix for first problem. The second is big
> dilemma. As I see in our projects, as they grow bigger and bigger, we
> faced many problems around the integer values. In this case the
> easiest way - and the compatible way - is to convert everything to
> integer (and #define the values).

Right.  But do you really need to turn the enum's into #define's?  Why?
Assigning an enum value to an int is never a problem in C++, right?

Btw, I have asked on the gnulib list for advise on the change:
http://thread.gmane.org/gmane.comp.lib.gnulib.bugs/28704

Unless there are platforms where this modifies the ABI, or some other
strong reason against, I'll make the change for the *_strerror
functions.

> At long term the enum is the better way. The consistency that the
> compiler provides for enums is great, but I think you know that.  I
> think the soultion is that idna_rc should be changed to int in the
> next minor release for all *_strerror, but in the future at as code
> reaches the next major version, the API can be rewritten, to use enums
> everywhere. As the result everyone can use the old version without
> code change, or the new next major version, with some minimal code
> rewrite.

Using 'int' now seems simplest indeed.  I'm not planning any API rewrite
of Libidn, so that alternative may never materialize.

/Simon

>
> On 2011.10.19. 11:18, Simon Josefsson wrote:
>> Bittner Ede <address@hidden> writes:
>> 
>>> Dear Development team,
>>> 
>>> I've tasked at my workplace, to use libidn. I've found some
>>> minor issues with the library 1, the idn-free function (in
>>> idn-free.h) misses the EXTERN "C" -qualifier, so when I included
>>> in the C++, I've get a link error. My workaround was to include
>>> the file with extern "C", but I think it isnt the preferable way,
>>> because all other header in the library do not need this. I've
>>> understand the comaplaints about the idn_free, but our production
>>> enviroment requires cross platform compatible code, so normal
>>> free is not an option for us.
>> 
>> Hello Bittner.  Thank you for your attention to detail and your
>> report.
>> 
>> It was a mistake, idn-free.h should also have the extern "C"
>> marker.  I have fixed this, it will be part of the next release.
>> 
>>> 2, all of the idna_* function return int value, but the
>>> idna_strerror takes the Idna_rc enum type as it's input
>>> parameter, so I've to static_cast the return value. This can
>>> easily avoided if the function returns Idna_rc instead of int.
>>> (And it will improve code quality as well)
>> 
>> This will require changing almost all APIs in the library...  I'm a
>> bit hesitant to do that -- it seems to me as if that would trigger
>> a lot of warnings in existing Libidn applications that use 'int'
>> for the return codes and now have to change to use 'Idna_rc' to
>> avoid a similar warning as you noticed.  Right?
>> 
>> How about if we make idna_strerror (and the other *_strerror
>> functions) take an int argument instead?  Then at least all library
>> functions are type-equal when it comes to error codes.
>> 
>> Cheers, /Simon