On Fri, May 3, 2013 at 3:24 PM, C. Michael Pilato <cmpilato_at_collab.net> wrote:
> Paul,
>
> Was reviewing your svn_client_log5() changes. There are a couple of places
> in your reworked svn_client_log5() code (resolve_log_targets(),
> specifically) that read like so or similar:
>
> if (peg_revision->kind == svn_opt_revision_unspecified)
> (*peg_revision).kind = svn_opt_revision_head;
>
> Is there any reason for that not to be simply:
>
> if (peg_revision->kind == svn_opt_revision_unspecified)
> peg_revision->kind = svn_opt_revision_head;
>
> ?
Hi Mike,
Sorry for the tardy reply. A weekend, a day off, and then some Ruby
binding investigation that "will only take an hour" delayed me :-\
Re the peg_revision, not sure what I wrote it that way. This refactor
went through a lot of iterations and somewhere along that line I
introduced that oddity, but now can't recall why it was or if, more
likely, it was just a mistake. Your suggestion is correct. Changed
in r1480723.
> Also, I would suggest that, instead of initializing the *relative_targets
> array with a single slot, we go ahead and use the number of slots we know
> we'll need (so we can avoid resizing the array later):
>
> /* Initialize the output array. At a minimum, we need room for one
> (possibly empty) relpath. Otherwise, we have to hold a relpath
> for every item in TARGETS except the first. */
> *relative_targets = apr_array_make(result_pool,
> MAX(1, targets->nelts - 1),
> sizeof(const char *));
Agreed, also changed in r1480723.
> Finally, do we need to be strdup()ing the stuff we put into the
> relative_targets array? Looks to me (via code inspection) like perhaps not.
You're right, it's not necessary as the code is written today. I was
only being overly cautious, thinking that we don't know what the
caller might do with the memory TARGETS is allocated in, so we should
make a copy of items put in the *RELATIVE_TARGETS output parameter. A
good practice for an API or even a Subversion-private function, but
yes, I see that maybe it's overkill for a static helper...
...On the other hand, we sometimes need to allocate the other output
parameter *RA_TARGET in RESULT_POOL[1] and we certainly need to
allocate *RELATIVE_TARGETS in RESULT_POOL. Personally I prefer that
hash/array output parameters have their contents allocated in a result
pool. My way uses more memory, but in the future any changes to the
caller don't need to worry about what might happen to TARGETS. Your
way uses memory more efficiently, but entangles resolve_log_target's
implementation with it's caller's. Curses...I've convinced myself
that both approaches are correct :-P I'd be happy to hear others
philosophy on this!
[1] Which admittedly is the same pool as SCRATCH_POOL right now.
> It's entirely possible that you didn't introduce any of this stuff with your
> recent code reorg here -- I'm not claiming otherwise -- but I wanted to make
> sure you agreed with me before I started messing around in that relatively
> complex bit of code.
>
> --
> C. Michael Pilato <cmpilato_at_collab.net>
> CollabNet <> www.collab.net <> Enterprise Cloud Development
>
--
Paul T. Burba
CollabNet, Inc. -- www.collab.net -- Enterprise Cloud Development
Skype: ptburba
Received on 2013-05-09 19:00:22 CEST