On Fri, Dec 15, 2000 at 01:19:38PM -0500, Jim Blandy wrote:
>
> There are two questions here:
>
> - Is const an effective safety mechanism?
>
> If you use const in a way that allows the compiler to verify that
> the invariants you want actually hold, and warn you when they don't,
> then, yes. I'm all in favor of using const this way. If I've
> missed such a case in the code, please point it out.
I agree, and will do.
> But if you use const to hint at what you mean --- say, by making
> svn_fs__list_length's argument `const skel_t *', while
> skel->children and skel->next are still mutable --- then it is not
> an effective safety mechanism. If someone adds code which whacks a
> skel node that was supposed to be left alone, the compiler probably
> won't warn you. This half-hearted style of const usage doesn't give
> you the "solid measure of safety" you're going for at all. To the
> contrary, it's more like a thin layer of ice --- who knows whether
> it can hold your weight?
I generally agree here, too. I think adding the const is a nice signal to
the user of the API, so it is beneficial. However, it doesn't provide the
safety (similar to casting away a const, which I mentioned in my email).
> - Should skels be read-only data structures?
>
> I hadn't intended them to be. My thought with skel.[ch] was to lay
> out a simple structure, describe a minimal set of invariants,
> provide some utility functions which expect and preserve those
> invariants, and beyond that, leave it up to the caller to use them
> correctly.
>
> I believe the current code does this. Skels don't fly around willy-
> nilly, as you seem to think.
I don't think that I used the term "willy nilly" :-), but they *are* used as
a basic interchange format. I just did a search for skel_t in libsvn_fs/*.h
and found quite a few in the APIs. They seem to be a rather basic data
structure.
> The functions which build atoms from
> literal strings are all in a position to ensure that the skel is
> used correctly.
Well, this is the coupling that I was talking about it. "used correctly" is
the hope :-)
> The skel is immediately written to a database
> record; it never passes outside the function, except to the skel
> utility functions, whose behavior is well-known.
Well, it appears a bit more than that, but then... I'm only seeing their
usage at a very peripheral level. But whether or not, the interesting part
is your next paragraph:
> I think you're arguing that the entire tree of skel nodes should be
> const --- that is, skel->data, skel->children, and skel->next would
> all refer to const objects,
Bing! And I tried this, too. It actually works out quite well.
> and the code should be changed to use
> constructor-style manipulation.
I'm not sure what exactly you mean by "constructor-style manipulation." The
issue is simply that [in a "all skels are const" approach] you can't alter
the CHILDREN or NEXT fields to hook them up. This effectively means that you
have to properly set the fields before returning the "const skel_t *".
Mostly, the only change is when lists are constructed. Currently, this is
typically done by creating and empty list, followed by a set of prepend
calls. I could see a variadic function to create a list from the N args.
I'm thinking something like:
list = make_list(pool, "ssSm", cstr1, cstr2, svnstr1, ptr, len);
[ and yes: if I suggest it, then I'm also signing up to write it if need be ]
> I can see some real advantages to
> this. You'd be shifting from the `hinting to the reader' style to
> thea `compiler can check' style, which addresses the shortcomings
> I've complained about before.
Yes!
In a large system, it is easier on this tired old noggin to see a const in a
prototype (or elsewhere) and feel assured that my data won't be changed
unexpectedly. The use of const is also handy for the flexibility (as I
mentioned before), in that both const and non-const data can be passed to
functions labeled as const.
> The only casting necessary would be
> in the functions that actually construct the skels to begin with,
> which is reasonable.
Yup. But even those guys don't necessarily need casting. They can construct
a "skel_t *" and then return it as "const skel_t *".
> However, it would make read / modify / write code, like that in
> tree.c:txn_body_change_node_prop, impossible; it would become read /
> reconstruct / write code. Perhaps this isn't a big loss.
That is the most-affected part of the move to "all const." I didn't apply
enough brain cells to figure out how it could be changed.
> In any case, this change should wait until after M2.
Well, there are a couple answers to this one: generally true because we
already have enough to do :-), but also that you or I don't have to make the
change. If one of the other committers has free time and can competently
make the change, then I don't see a problem with adding it for M2.
[ I used to get on Guido's case about this all the time. At one point, he
said "we aren't going to ANSI-ify the function definitions for this next
release -- I don't have time." I responded simply with, "*You* don't, but
*we* do." Over the next week, several people poured through the code and
revamped it from K&R defns to ANSI defns. ]
> Setting the question of skel tree structure aside, I may agree with
> you that skel->data should be const. I can't think of any situation
> where it is appropriate to change *skel->data. Since the atom's text
> is usually a substring in a larger object, you'd have to be dealing
> with fixed-width data, which is pretty rare, and usually a bad idea to
> begin with. This would clean up the string literal issue as well.
Agreed.
Cheers,
-g
p.s. and the irony is that I prefer Python over C
p.p.s. you've got to understand Python's typing and the phrase, "we're all
adults here" to see the irony :-)
--
Greg Stein, http://www.lyra.org/
Received on Sat Oct 21 14:36:17 2006