[gnutls-dev] Re: constification patch

[Simon Josefsson]

"ZIGLIO, Frediano, VF-IT" <Frediano.Ziglio at vodafone.com> writes:

>> 
>> "ZIGLIO, Frediano, VF-IT" <Frediano.Ziglio at vodafone.com> writes:
>> 
>> > This simple patch add some const definition. It also assure 
>> that global
>> > variables are really const.
>> 
>> I installed most of the patch.
>> 
>> The lib/minitasn1/ changes didn't look right, so I didn't install
>> them.  First, 'inline' can't be used because libtasn1 is supposed to
>> be C89 portable.  Also, isn't it possible to solve this without
>> creating new *_const functions?  Moving typecasts into a special
>> function just hide the problems.  I think warnings are better here
>> because then we know that there is a problem.
>> 
>> Thanks,
>> Simon
>> 
>
> Hi Simon,
>   I would never expected that you ever apply the patch so fast!
>
> Well, I came from "C++ world" were const is very important but easier to
> use than in C however after some year developing OpenSource C
> applications I have to say that const is important even on C... The
> *_const functions reflect how C++ work... but in C. In C++ you can
> declare two functions like
>
>   char *strchr(char *, int);
>   const char *strchr(const char *, int);
>
> without problems, in C this give error... so I added _const. The
> advantage is that compiler help you. Usually many people do not pay many
> attention to warning but some time ignoring warnings can lead to
> disaster... so usually I remove any type of warning I found. In this
> case I call a function for no-const object knowing that this function do
> not change the data.
>
> Well, after explaining my reasons let's came to a possible (and
> portable) fix. A way to fix this issue is to use static inline for
> compilers that support this or define for others. As I think that many
> people will use gcc so they will see the error I would convert 
>
> static inline const node_asn *
> _asn1_find_up_const(const node_asn *node)
> {
> 	return _asn1_find_up((node_asn*) node);
> }
>
> to 
>
> #if defined(__GNUC__) && __GNUC__ >= 3
> static inline const node_asn *
> _asn1_find_up_const(const node_asn *node)
> {
> 	return _asn1_find_up((node_asn*) node);
> }
> #else
> #define _asn1_find_up_const(node) _asn1_find_up((node_asn*) (node))
> #endif

How about changing the prototype of _asn1_find_up to include the const
keyword instead?  That seem more correct.

Using typecasts like you do hide problems: what if _asn1_find_up does
not preserve const?  That would lead to a disaster, and the compiler
will not warn about it.

Further, it has not been written down, but the coding style for
minitasn1 is slightly different than GnuTLS.  Minitasn1 should be
portable C89 code, and #if's like that make the code unreadable.
Unless profiling show that you'd gain a non-negligible amount of time
making that particular function inline (I highly doubt that), then it
is worth the extra CPU cycles to have readable code.  Optimize only
when you need.  In contrast, in GnuTLS we can use 'inline'
unconditionally because there is a gnulib M4 test that make sure
'inline' is defined to "" if it is not available.

If you update the patch, to change the prototype of the existing
functions to use 'const' too, I'll review it.  I know there is a lot
of missing 'const' keywords in various places.

Thanks,
Simon