You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Ramkumar Ramachandra <ar...@gmail.com> on 2010/09/21 18:54:30 UTC
Re: svn commit: r999507 - in /subversion/trunk/subversion:
svnrdump/dump_editor.c tests/cmdline/svnrdump_tests.py
Hi Daniel,
Thanks for the review.
Daniel Shahaf writes:
> > - /* Disallow a path relative to nothing. */
> > - SVN_ERR_ASSERT_NO_RETURN(!path || pb);
> > -
>
> Why did you drop this assertion?
I thought about it for a bit, and I can't seem to figure out why I
added it in the first place. My current sanity logic: Without a pb,
this function (make_dir_baton) generates a root baton. Whatever `path`
the user passes is ignored anyway- the root baton has `abspath` set to
`/`. With a `pb`, I MUST pass a non-null `path` -- there's probably
something about this I haven't thought about properly that's making
test 14 fail (there's an empty Node-path there). Either way, I don't
see where an assert fits into the picture.
> Also; if you intend to move it back, SVN_ERR_ASSERT() would be better
> (for when this code becomes a library function).
Ok, I'll keep that in mind when I write more asserts.
> > /* Construct the full path of this node. */
> > if (pb)
> > abspath = svn_uri_join("/", path, pool);
> > else
> > - abspath = "/";
> > + abspath = apr_pstrdup(pool, "/");
> >
>
> It is most likely unnecessary to duplicate a static string constant. :-)
True. However, abspath is a `char *` with no memory allocated to it: I
want whatever its value is to be allocated in `pool` since it's passed
around.
-- Ram
Re: svn commit: r999507 - in /subversion/trunk/subversion:
svnrdump/dump_editor.c tests/cmdline/svnrdump_tests.py
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi,
Daniel Shahaf writes:
> abspath is a pointer, lives on the stack, and is passed by value (like
> any old int) in function calls. What you allocate from the pool is the
> value it points to --- i.e., the two bytes of "/".
>
> However, literal string constants are static strings --- they are loaded
> into memory from the binary image (ever tried 'strings /usr/bin/svn'?)
> and stay there until that is unloaded --- while pool-allocated strings
> allocated on the heap, and stays there until the pool is cleared.
> The other difference is that literal strings are not writable (they are
> const char *) while pool-allocate strings are mutable (non-const char *).
>
> (waiting to hear from you that dropping the strdup() causes a segfault)
Hehe. See my other email- don't worry, I wasn't confused about static
and dynamic allocation :p I just got muddled up in my whole "elegance"
argument.
> So, the only reasons you'd have to duplicate a static string is if the
> static string lives in a library which you know you'll unload[1] or if
> you need a non-const char * for some reason. In your case, 'abspath' is
> a const char *, so neither of these cases applies.
Right, it's been justified then- I was being silly.
> Okay?
Thanks for the explanation. Will drop during the cleanup.
-- Ram
Re: svn commit: r999507 - in /subversion/trunk/subversion:
svnrdump/dump_editor.c tests/cmdline/svnrdump_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
Ramkumar Ramachandra wrote on Thu, Sep 23, 2010 at 01:12:52 +0530:
> Daniel Shahaf writes:
> > Choosing path=NULL to represent "the root" seems odd to me (that's not
> > how we usually represent it), but that's for another day.
>
> I think I'm using "" to represent the root somewhere, but I'll have to
> check this. I'll figure something out during the cleanup.
>
I see.
> > > True. However, abspath is a `char *` with no memory allocated to it: I
> > > want whatever its value is to be allocated in `pool` since it's passed
> > > around.
> >
> > I think you've got your pointers confused. Can you drop the pstrdup(),
> > see that nothing breaks, and then we can discuss why nothing broke?
>
> Hehe, I wouldn't be surprised if this is the case -- I've been working
> with APR pools day and night now :p
>
> Okay, I'll drop this during the cleanup. Do explain this to me though :)
abspath is a pointer, lives on the stack, and is passed by value (like
any old int) in function calls. What you allocate from the pool is the
value it points to --- i.e., the two bytes of "/".
However, literal string constants are static strings --- they are loaded
into memory from the binary image (ever tried 'strings /usr/bin/svn'?)
and stay there until that is unloaded --- while pool-allocated strings
allocated on the heap, and stays there until the pool is cleared.
The other difference is that literal strings are not writable (they are
const char *) while pool-allocate strings are mutable (non-const char *).
So, the only reasons you'd have to duplicate a static string is if the
static string lives in a library which you know you'll unload[1] or if
you need a non-const char * for some reason. In your case, 'abspath' is
a const char *, so neither of these cases applies.
Okay?
Daniel
(waiting to hear from you that dropping the strdup() causes a segfault)
[1] Thanks, dev@ archives.
Re: svn commit: r999507 - in /subversion/trunk/subversion:
svnrdump/dump_editor.c tests/cmdline/svnrdump_tests.py
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi again,
Ramkumar Ramachandra writes:
> > I think you've got your pointers confused. Can you drop the pstrdup(),
> > see that nothing breaks, and then we can discuss why nothing broke?
>
> Hehe, I wouldn't be surprised if this is the case -- I've been working
> with APR pools day and night now :p
>
> Okay, I'll drop this during the cleanup. Do explain this to me though :)
Just to be clear: I know that nothing will break. Memory for the
string "/" will be allocated statically by the compiler- its lifetime
is therefore the lifetime of the program, and it cannot be freed
before that. I made this change merely for the purpose of elegance-
except in this case, abspath always points to a dynamically allocated
memory location owned by a `pool` that can be freed. So I thought: why
not make make it always point to a dynamically allocated memory
location?
Does that make sense or am I just being silly? :p
-- Ram
Re: svn commit: r999507 - in /subversion/trunk/subversion:
svnrdump/dump_editor.c tests/cmdline/svnrdump_tests.py
Posted by Ramkumar Ramachandra <ar...@gmail.com>.
Hi Daniel,
Daniel Shahaf writes:
> It actually makes sense: "either there is a parent baton, or I'm
> creating the root baton (and therefore the caller passed path=NULL)".
>
> Choosing path=NULL to represent "the root" seems odd to me (that's not
> how we usually represent it), but that's for another day.
Ah. I guess that explains it was there in the first place :p
I think I'm using "" to represent the root somewhere, but I'll have to
check this. I'll figure something out during the cleanup.
> > True. However, abspath is a `char *` with no memory allocated to it: I
> > want whatever its value is to be allocated in `pool` since it's passed
> > around.
>
> I think you've got your pointers confused. Can you drop the pstrdup(),
> see that nothing breaks, and then we can discuss why nothing broke?
Hehe, I wouldn't be surprised if this is the case -- I've been working
with APR pools day and night now :p
Okay, I'll drop this during the cleanup. Do explain this to me though :)
-- Ram
Re: svn commit: r999507 - in /subversion/trunk/subversion:
svnrdump/dump_editor.c tests/cmdline/svnrdump_tests.py
Posted by Daniel Shahaf <d....@daniel.shahaf.name>.
[ sorry for the delayed reply. ]
Ramkumar Ramachandra wrote on Wed, Sep 22, 2010 at 00:24:30 +0530:
> Daniel Shahaf writes:
> > > - /* Disallow a path relative to nothing. */
> > > - SVN_ERR_ASSERT_NO_RETURN(!path || pb);
> > > -
> >
> > Why did you drop this assertion?
>
> I thought about it for a bit, and I can't seem to figure out why I
> added it in the first place. My current sanity logic: Without a pb,
> this function (make_dir_baton) generates a root baton. Whatever `path`
> the user passes is ignored anyway- the root baton has `abspath` set to
> `/`. With a `pb`, I MUST pass a non-null `path` -- there's probably
> something about this I haven't thought about properly that's making
> test 14 fail (there's an empty Node-path there). Either way, I don't
> see where an assert fits into the picture.
>
It actually makes sense: "either there is a parent baton, or I'm
creating the root baton (and therefore the caller passed path=NULL)".
Choosing path=NULL to represent "the root" seems odd to me (that's not
how we usually represent it), but that's for another day.
> > > /* Construct the full path of this node. */
> > > if (pb)
> > > abspath = svn_uri_join("/", path, pool);
> > > else
> > > - abspath = "/";
> > > + abspath = apr_pstrdup(pool, "/");
> > >
> >
> > It is most likely unnecessary to duplicate a static string constant. :-)
>
> True. However, abspath is a `char *` with no memory allocated to it: I
> want whatever its value is to be allocated in `pool` since it's passed
> around.
I think you've got your pointers confused. Can you drop the pstrdup(),
see that nothing breaks, and then we can discuss why nothing broke?
:-)
Daniel
>
> -- Ram