[gnutls-dev] RE: constification patch

[ZIGLIO, Frediano, VF-IT]

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

Yes and not... you end up changing some const* to no const in
_asn1_find_up... the reason is that for const node_asn* I mean that all
ASN tree is constant so is a bad idea to return a node_asn *...

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

Perhaps it would be more polite if: 

we define a _asn1_find_up_const function in C source (renaming
_asn1_find_up to const) which take const and return const and a
_asn1_find_up function that take no-const and return no-const (a wrapper
to previous one). This would also allow easy detection of invalid
changes in _asn1_find_up_const.

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

No problem for optimizazion, if we use inline only for gcc gcc can do a
good work. Using define however do not check for const/no-const (C don't
have a const_cast like C++...). I don't find however a solution that can
be portable and readable as you want and also optimized and with
"efficient" warnings... but we can catch three of them (portable,
readable, clean warning) with a "normal" _asn1_find_up_const function
(no static, no inline, in source file with declaration).

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

Ok I'll look a bit deeper. I have to CVS direct access here so I'll have
to wait tomorrow to integrate my patch...

Frediano Ziglio