const-ness

Omitting the use of "const" because it "doesn't say enough" is a bit bogus
in my mind. Even though it might only refer to the top level, it still tells
the reader, "I'm not planning to change this." Sure, it could change
children, but I'd consider the top-level const a lie in that case :-)

A parameter that has "const" on it can take "const type *" or "type *"
values. If you drop the const, then you cannot take a const argument. You
must insert a cast.

So the developer responds with: "But the function isn't really going to
change it, so I know that casting away the const is safe."

Great. So you must know an awful lot about the internals of that function.
Can you say "coupling"? By omitting the cast, it is now possible that
somebody down the line *can* modify the argument. Oh sure... they really
aren't supposed to do that, but in a complex system, it is going to happen.
You've lost your safety net, and you're going to get a segfault because
somebody passed in some truly const data.

Part of the addition of const last month was because we had stuff that was
*truly* const. If some code tried to change it... BAM. These came in two
classes: string constants, and static-const (prebuilt) ID structures.

The basis for this recent reversal of const is the premise that "the DATA in
a skel can be changed." I find nothing to support such a premise. It is also
a *very* scary proposition. All too often, we insert constant strings, such
as "revision", into that data field. Oh, maybe sometimes they come from the
database and they're allocated in memory. But not always. And that one time,
some code is going to go ahead and change the data and BAM.

Skels should be totally const. Always. The data, the children, and the next
pointer. Build them bottom-up, and return them as const.

As a test, I just went and added "const" to the data, children, and next
fields in the skel_t structure. I also changed most of the functions to
accept/return const skels. Then I propagated const as appropriate. There are
only three issues that resulted:

1) the use of svn_fs__prepend() for building skels. a flow change in these
   areas would be needed (to construct from bottum-up, rather than in a
   top-down manner).

2) txn_body_change_node_prop() fetches a skel, changes its, and puts it
   back. this one is a bit tricky :-). a cast certainly solves it, but that
   implies that we know svn_fs__dag_get_proplist() returns us a skell that
   we can/should change.

3) skel.c: list() and svn_fs__copy_skel() have some minor issues based on
   the way they construct the list/copy. these can safely use casts to toss
   the const (since they're construction mechanisms).

Use the const throughout the code adds a solid measure of safety, and
enhances the descriptiveness of the code ("I'm not going to change this
argument" becomes very obvious).

I'd rather not rely on comments, and some of the coupling that is created by
casting away const markers. Applying a uniform policy of const-ness will
give us much more safety, readability, and maintainability.

I'm happy to post my const demo patch if anybody is interested. It isn't
complete (mostly the prepend thing), but gives an idea of what is involved
with const-ifying skels.

Cheers,
-g

-- 
Greg Stein, http://www.lyra.org/