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 2001/02/16 02:10:30 UTC
Re: CVS update: subversion/subversion/libsvn_fs tree.c Makefile.am
On Thu, Feb 15, 2001 at 11:17:58PM -0000, jimb@tigris.org wrote:
>...
> --- tree.c 2001/02/15 18:58:53 1.16
> +++ tree.c 2001/02/15 23:17:58 1.17
>...
> +static svn_error_t *
> +mutable_root_node (dag_node_t **node_p,
> + svn_fs_root_t *root,
> + const char *error_path,
> + trail_t *trail)
> +{
> + /* If it's a revision root, we can't change its contents. */
> + if (root->rev != -1)
> + return svn_fs__err_not_mutable (root->fs, root->rev, error_path);
What's that magic number there? SVN_INVALID_REVNUM? Something else?
Can you use a symbol rather than a free-floating number?
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: CVS update: subversion/subversion/libsvn_fs tree.c Makefile.am
Posted by Greg Stein <gs...@lyra.org>.
On Fri, Feb 16, 2001 at 11:02:05AM -0500, Jim Blandy wrote:
>
> Greg Stein <gs...@lyra.org> writes:
> > On Thu, Feb 15, 2001 at 11:17:58PM -0000, jimb@tigris.org wrote:
> > >...
> > > --- tree.c 2001/02/15 18:58:53 1.16
> > > +++ tree.c 2001/02/15 23:17:58 1.17
> > >...
> > > +static svn_error_t *
> > > +mutable_root_node (dag_node_t **node_p,
> > > + svn_fs_root_t *root,
> > > + const char *error_path,
> > > + trail_t *trail)
> > > +{
> > > + /* If it's a revision root, we can't change its contents. */
> > > + if (root->rev != -1)
> > > + return svn_fs__err_not_mutable (root->fs, root->rev, error_path);
> >
> > What's that magic number there? SVN_INVALID_REVNUM? Something else?
> > Can you use a symbol rather than a free-floating number?
>
> Well, there's only one distinguished value for the `rev' field of an
> svn_fs_root_t. It's documented in the explanation of that field. -1
> is a common quiescent value for quantities for which zero is
> meaningful --- like revision numbers. And there's a comment directly
> above that `if', explaining what it's checking for. So I think the
> code is pretty legible as it stands.
Because of the simple fact that I asked, empirical evidence would say that
the code is /not/ legible.
The comment regarding -1 is a long ways away from the usage, so it is easy
for a person to not know about it (e.g. my case). Further, how would a
person know that -1 represents "uninitialized" or "quiescent" rather than a
meaningful value? I certainly didn't understand that the -1 meant "no
meaning here, just move along."
Bare numbers are always a bit of a problem because the reader can never
truly understand whether they are significant and/or their semantics.
Cheers,
-g
p.s. yes, I know this concrete case doesn't apply; I'm dicussing style now :-)
--
Greg Stein, http://www.lyra.org/
Re: CVS update: subversion/subversion/libsvn_fs tree.c Makefile.am
Posted by Jim Blandy <ji...@zwingli.cygnus.com>.
Greg Stein <gs...@lyra.org> writes:
> On Thu, Feb 15, 2001 at 11:17:58PM -0000, jimb@tigris.org wrote:
> >...
> > --- tree.c 2001/02/15 18:58:53 1.16
> > +++ tree.c 2001/02/15 23:17:58 1.17
> >...
> > +static svn_error_t *
> > +mutable_root_node (dag_node_t **node_p,
> > + svn_fs_root_t *root,
> > + const char *error_path,
> > + trail_t *trail)
> > +{
> > + /* If it's a revision root, we can't change its contents. */
> > + if (root->rev != -1)
> > + return svn_fs__err_not_mutable (root->fs, root->rev, error_path);
>
> What's that magic number there? SVN_INVALID_REVNUM? Something else?
> Can you use a symbol rather than a free-floating number?
Well, there's only one distinguished value for the `rev' field of an
svn_fs_root_t. It's documented in the explanation of that field. -1
is a common quiescent value for quantities for which zero is
meaningful --- like revision numbers. And there's a comment directly
above that `if', explaining what it's checking for. So I think the
code is pretty legible as it stands.
In general, I don't think readibility is really served by introducing
names for every quiescent value in the system, when those values
follow established idioms.
But the whole way I was distinguishing revision and transaction roots
wasn't especially clean, so I've redone it in a better way. Thanks
for bringing it up.