const, the errors framework, and -Wcast-qual

[ghudson-2]

My C experience leads me to believe that const pointers should be used
for (1) pointers to non-heap-allocated memory (C string literals,
stack memory, etc.) which you don't want changed, or (2) aliases to
heap-allocated memory owned by some other, non-const pointer, which
you don't want changed through the alias.  In particular, it's my
understanding that const pointers should *not* be used for pointers
which will be used to free memory; destroying is a form of
modification, so if a pointer owns a piece of heap-allocated memory,
it has to be non-const even if that memory is intended to be immutable
until freed.

The errors framework doesn't agree with this principal.  struct
errinfo contained a const pointer to memory it owns, although I just
changed that.  krb5int_get_error and krb5_get_error_message return
const pointers even though those pointers are used to free memory.
The latter function is part of our API, although I don't know if it
would be a problem to change it to return char *, since that shouldn't
break any calling code.

I don't want to go on a crusade if I'm out of step with the community
on this.  I can find stuff like
http://bytes.com/topic/c/answers/217935-passing-const-void-free (which
is more of an argument than a refernce) and
https://barnowl.mit.edu:444/wiki/const (which can't possibly be
authoritative since it was written by someone yonuger than I am), but
no Voice of Authority.  So, here is a change to tell me that I am
educated stupid about this.

On a related and equally nitpicky note, I would like to remove
-Wcast-qual from our gcc warning set.  Because we make heavy use of
krb5_data in our code base, we have a lot of cases where we
deliberately de-constify a pointer in order to fit into a krb5_data
value for use by some function which won't modify the memory (like
input to krb5_c_encrypt).  I don't think it does us a big service to
identify places where people are deliberately circumventing const for
a reason such as this, when we usually can't fix it.
_______________________________________________
krbdev mailing list             krbdev@...
https://mailman.mit.edu/mailman/listinfo/krbdev

[Ken Raeburn]

On Nov 1, 2009, at 17:55, ghudson@... wrote:
> The errors framework doesn't agree with this principal.  struct
> errinfo contained a const pointer to memory it owns, although I just
> changed that.  krb5int_get_error and krb5_get_error_message return
> const pointers even though those pointers are used to free memory.

The intent was that "application" code shouldn't be allowed to change  
the string, but the library code should be allowed to free it.  It's  
not easy to express that with both parts of the code in C unless you  
cast away const or you keep around a non-const version of the pointer.

We did wind up changing the design once or twice in the process.  This  
might've been something that worked better in the earlier design.

I'd have no objection to changing it, since we're now always returning  
writable copies of the error string, and usually independent copies.  
(We were considering letting get_error work multiple times, which can  
get complicated if allocation fails so some of the copies have to  
share the scratch buffer; in that case, you don't want the application  
to have been written to modify the returned string.  But we never made  
that part of the official interface.)

> On a related and equally nitpicky note, I would like to remove
> -Wcast-qual from our gcc warning set.  Because we make heavy use of
> krb5_data in our code base, we have a lot of cases where we
> deliberately de-constify a pointer in order to fit into a krb5_data
> value for use by some function which won't modify the memory (like
> input to krb5_c_encrypt).  I don't think it does us a big service to
> identify places where people are deliberately circumventing const for
> a reason such as this, when we usually can't fix it.

It's a shame we can't define a krb5_const_data with automatic pointer  
conversions and guaranteed compatibility...

Ken
_______________________________________________
krbdev mailing list             krbdev@...
https://mailman.mit.edu/mailman/listinfo/krbdev

[Sam Hartman]

I agree that for non-refcounted objects that freeing a const pointer
is bad.

In the case of objects with refcounts where you may want to ask some
people not to mutate the object, I think freeing through a const
pointer when the last reference goes away is better than vere using
const pointers for such aliases.  That situation does not come up in our code base.

I also think that const pointers are justified when they sometimes
point to shared storage (like error strings) that it would be highly
undesirable to change even if that meant freeing through a const
pointer.

What I think I'm saying is that Anders/your rule of don't free through
const is probably too strong as a rule,but is a good guidline.  I
agree with you that applying that guideline in the cases you're
thinking of would improve the code.

--Sam
_______________________________________________
krbdev mailing list             krbdev@...
https://mailman.mit.edu/mailman/listinfo/krbdev

[ghudson-2]

To close out this thread:

* I removed -Wcast-qual from the standard gcc warnings set.

* I'm not going to unconstify the errors framework further, because I
realized that it would be pointless to change krb5_get_error_message
without also changing krb5_free_error_message, and changing the latter
to take a char * would be incompatible with existing code.


_______________________________________________
krbdev mailing list             krbdev@...
https://mailman.mit.edu/mailman/listinfo/krbdev