You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Stein <gs...@lyra.org> on 2000/12/15 12:36:43 UTC
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/
Re: const-ness
Posted by Greg Stein <gs...@lyra.org>.
On Sat, Dec 16, 2000 at 07:07:31AM +0100, Branko Cibej wrote:
> Greg Stein wrote:
> > 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);
>
> No no no no! I'm shocked that you're talking about adding "const" for
> safety reasons and variadic functions in the same breath.
Heh. :-)
The problems aren't very similar. The "const" problem is to prevent coupling
across a broad set of functions/modules. A variadic function does not
introduce the same kind of coupling, so there is a *much* smaller chance for
unintended side effects.
Yes, it is possible to pass incorrect params to a format-string-based
variadic, but you'll probably see them the first time you hit that line of
code. Once that line is correct, it is very difficult to break it.
Dropping "const" from a system opens up holes for problems. The code is
"looser", if you will, and is less robust. Change some in X, and an
assumption that Y made about "well, it won't change it" could be blown away.
> > [ and yes: if I suggest it, then I'm also signing up to write it if need be ]
>
> Ah, then you're also signing up to debug all wrong usage that'll crop
> up. That's fine then, go ahead. :-)
For this? Sure. I doubt we'll see problems with this code pattern.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: const-ness
Posted by Branko Čibej <br...@xbc.nu>.
Greg Stein wrote:
> 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);
No no no no! I'm shocked that you're talking about adding "const" for
safety reasons and variadic functions in the same breath.
> [ and yes: if I suggest it, then I'm also signing up to write it if need be ]
Ah, then you're also signing up to debug all wrong usage that'll crop
up. That's fine then, go ahead. :-)
--
Brane �ibej
home: <br...@xbc.nu> http://www.xbc.nu/brane/
work: <br...@hermes.si> http://www.hermes-softlab.com/
ACM: <br...@acm.org> http://www.acm.org/
Re: const-ness
Posted by Greg Stein <gs...@lyra.org>.
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/
Re: const-ness
Posted by Karl Fogel <kf...@galois.collab.net>.
Greg Stein <gs...@lyra.org> writes:
> That's the theory, at least. It's a fine line, but when it gets down to
> brass tacks, the FS follows Jim's design. I can say there are problems, but
> can also respect it when Jim puts his foot down :-) Of course, I hope to
> receive the same treatment: tell me about problems in my code, and we can
> work through it; at some point, if it is six of one, half dozen of another
> for the two approaches, then I'd hope to get the deciding vote :-).
Absolutely.
> I sense a bit of worry, Karl :-). No need... I know how to share a sandbox
> with the other boys :-)
Sorry -- I saw the const discussion going on longer than [I] expected,
and overreacted. My bad; it's abundantly clear that you're a sandbox
pro. :-)
Re: const-ness
Posted by Greg Stein <gs...@lyra.org>.
On Fri, Dec 15, 2000 at 11:32:33AM -0600, Karl Fogel wrote:
> Greg, re the const stuff: it sounds like you're advocating a certain
> coding policy, but not succeeding in convincing a majority of the
> other developers that it's a good policy :-).
Heh :-)
It isn't my job to "convince" anybody. It would be a bit presumptuous of me
to think I knew better than somebody else. Also, the primary design and
development of the code bits fall to different people, and they get to make
the ultimate design choices and decisions.
I try to avoid the "convincing" and (instead) just take my job as "review
the code and flag/describe problems as I see them; to provide alternate
code/designs that I think are better."
That's the theory, at least. It's a fine line, but when it gets down to
brass tacks, the FS follows Jim's design. I can say there are problems, but
can also respect it when Jim puts his foot down :-) Of course, I hope to
receive the same treatment: tell me about problems in my code, and we can
work through it; at some point, if it is six of one, half dozen of another
for the two approaches, then I'd hope to get the deciding vote :-). If there
is a problem in my design/code, then I hope people can explain it to me so
we can create a better system.
> Since coding policies only exist to help humans (the code can get
> along quite fine without them), if the humans don't find a particular
> policy helpful, then there isn't much reason for them to adopt it.
> JimB is uncomfortable with reading more into a `const' than the
> qualifier technically promises. In the sections of code where he
> spends most of his time, it doesn't make sense for him be doing things
> that make him uncomfortable -- that'll just get in his way.
Agreed.
> My feeling is, you use const the way you like in the code you're
> writing, JimB will use it how he likes in the code he's writing, etc,
> and Subversion will simply not have a project-wide policy on const
> usage.
Certainly. While I might get a bit "loud" at times :-), I will always defer
to his choices for the FS.
I sense a bit of worry, Karl :-). No need... I know how to share a sandbox
with the other boys :-)
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: const-ness
Posted by Karl Fogel <kf...@galois.collab.net>.
Greg, re the const stuff: it sounds like you're advocating a certain
coding policy, but not succeeding in convincing a majority of the
other developers that it's a good policy :-).
Since coding policies only exist to help humans (the code can get
along quite fine without them), if the humans don't find a particular
policy helpful, then there isn't much reason for them to adopt it.
JimB is uncomfortable with reading more into a `const' than the
qualifier technically promises. In the sections of code where he
spends most of his time, it doesn't make sense for him be doing things
that make him uncomfortable -- that'll just get in his way.
My feeling is, you use const the way you like in the code you're
writing, JimB will use it how he likes in the code he's writing, etc,
and Subversion will simply not have a project-wide policy on const
usage.
-K
Jim Blandy <ji...@zwingli.cygnus.com> writes:
> 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.
>
> 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?
>
> - 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. The functions which build atoms from
> literal strings are all in a position to ensure that the skel is
> used correctly. 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.
>
> 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, and the code should be changed to use
> constructor-style manipulation. 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. The only casting necessary would be
> in the functions that actually construct the skels to begin with,
> which is reasonable.
>
> 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.
>
> In any case, this change should wait until after M2.
>
> 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.
Re: const-ness
Posted by Jim Blandy <ji...@zwingli.cygnus.com>.
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.
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?
- 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. The functions which build atoms from
literal strings are all in a position to ensure that the skel is
used correctly. 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.
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, and the code should be changed to use
constructor-style manipulation. 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. The only casting necessary would be
in the functions that actually construct the skels to begin with,
which is reasonable.
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.
In any case, this change should wait until after M2.
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.