You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by kf...@collab.net on 2003/12/26 01:27:27 UTC
Re: svn commit: rev 8081 - trunk/subversion/libsvn_repos
ghudson@tigris.org writes:
> +/* If authorized, emit a delta to create the entry named TARGET_ENTRY
> + at the location EDIT_PATH. If not authorized, indicate that
> + EDIT_PATH is absent. Pass DIR_BATON through to editor functions
> + that require it. */
> static svn_error_t *
> add_file_or_dir (struct context *c, void *dir_baton,
> const char *target_path,
> @@ -835,10 +789,19 @@
> apr_pool_t *pool)
> {
> struct context *context = c;
> + svn_boolean_t allowed;
>
> /* Sanity-check our input. */
> assert (target_path && edit_path);
>
> + if (c->authz_read_func)
> + {
> + SVN_ERR (c->authz_read_func (&allowed, c->target_root, target_path,
> + c->authz_read_baton, pool));
> + if (!allowed)
> + return absent_file_or_dir (c, dir_baton, edit_path, tgt_kind, pool);
> + }
I'd declare 'allowed' inside the if's body. Right now, a reader can
easily fear that 'allowed' might be used uninitialized later. Plus,
it's slightly easier to add code later which *does* use it
uninitialized, if one doesn't read the conditional structure
carefully.
I know it's not a matter of correctness, and anyone adding code
*should* read the conditionals carefully, of course. It's just nice
when code doesn't raise unnecessary questions/worries that then have
to be quelled.
(Yes, I remember we talked about these two styles on IRC :-) ).
> +/* If authorized, emit a delta to modify EDIT_PATH with the changes
> + from SOURCE_PATH to TARGET_PATH. If not authorized, indicate that
> + EDIT_PATH is absent. Pass DIR_BATON through to editor functions
> + that require it. */
> static svn_error_t *
> replace_file_or_dir (struct context *c,
> void *dir_baton,
> @@ -882,10 +846,19 @@
> apr_pool_t *pool)
> {
> svn_revnum_t base_revision = SVN_INVALID_REVNUM;
> + svn_boolean_t allowed;
>
> /* Sanity-check our input. */
> assert (target_path && source_path && edit_path);
>
> + if (c->authz_read_func)
> + {
> + SVN_ERR (c->authz_read_func (&allowed, c->target_root, target_path,
> + c->authz_read_baton, pool));
> + if (!allowed)
> + return absent_file_or_dir (c, dir_baton, edit_path, tgt_kind, pool);
> + }
Same comment applies here.
+1 on the change as a whole, though! This factorization feels very
right.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 8081 - trunk/subversion/libsvn_repos
Posted by Greg Hudson <gh...@MIT.EDU>.
On Sat, 2003-12-27 at 08:14, Tobias Ringström wrote:
> Greg Hudson wrote:
> > At any rate, there's no reason for programmers to spend any time
> > thinking about uninitialized variables. The compiler will find them.
> > Compilers can't be perfect about it, but gcc always errors on the side
> > of warnings you inappropriately, rather than failing to warn you
> > appropriately.
>
> Unfortunately it doesn't. If you take the address of the variable
> anywhere in the function, gcc's unused variable warning mechanism is
> disabled:
Fascinating.
My mail point holds (that is, I still think it's more important not to
break up code with declarations), but my sub-point obviously isn't
valid.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 8081 - trunk/subversion/libsvn_repos
Posted by Tobias Ringström <to...@ringstrom.mine.nu>.
Greg Hudson wrote:
> At any rate, there's no reason for programmers to spend any time
> thinking about uninitialized variables. The compiler will find them.
> Compilers can't be perfect about it, but gcc always errors on the side
> of warnings you inappropriately, rather than failing to warn you
> appropriately.
Unfortunately it doesn't. If you take the address of the variable
anywhere in the function, gcc's unused variable warning mechanism is
disabled:
void print(int x);
void foo(int *x);
void bar(void)
{
int i;
print(i);
foo(&i);
}
~> gcc -O -Wall -W -c tmp.c
~>
(gcc 3.3.2 and others)
/Tobias
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: rev 8081 - trunk/subversion/libsvn_repos
Posted by Greg Hudson <gh...@MIT.EDU>.
I committed:
> + if (c->authz_read_func)
> + {
> + SVN_ERR (c->authz_read_func (&allowed, c->target_root, target_path,
> + c->authz_read_baton, pool));
> + if (!allowed)
> + return absent_file_or_dir (c, dir_baton, edit_path, tgt_kind, pool);
> + }
Karl wrote;
> I'd declare 'allowed' inside the if's body. Right now, a reader can
> easily fear that 'allowed' might be used uninitialized later. Plus,
> it's slightly easier to add code later which *does* use it
> uninitialized, if one doesn't read the conditional structure
> carefully.
It took me a while to figure out that you meant that people might
think "allowed" is used later on in the function; I initially thought
you meant that people might think "allowed" isn't necessarily
initialized when used in the if statement. Moving the declaration
doesn't make that any more or less likely.
At any rate, there's no reason for programmers to spend any time
thinking about uninitialized variables. The compiler will find them.
Compilers can't be perfect about it, but gcc always errors on the side
of warnings you inappropriately, rather than failing to warn you
appropriately.
I can understand wanting to declare variables in inner scopes so that
the reader can more easily see when the variable is no longer
relevant. But I think it's more important not to break up code with
variable declarations. I hate reading code and having my internal
monologue go: "Okay, if there's an authorization function, we... okay,
here are some variable declarations... er, damn, where was I? Oh,
right, if there's an authorization function, we call it with these
arguments, and if it says no, we..."
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org