"Troy Curtis Jr" <troycurtisjr_at_gmail.com> writes:
>> Oh, I see: you're having the caller be responsible for detecting an
>> escaping '@'. That could work, but it would be a cleaner interface to
>> just have split_arg_at_peg_revision() take care of this, which is what
>> split_arg_at_peg_revision()'s doc string currently claims anyway.
>
> Well the doc string does explicity state that the '@' character is
> left in the peg revision parameter as the first character.
Yes, but that's only if there actually is a peg revision. If there is
no peg revision, then *peg_rev is supposed to be NULL. (IOW, assume
that your doc string is going to be taken absolutely literally. A "peg
revision" is a semantic thing -- it's not "anything, including the empty
string, following an @-sign at the end of a path").
>> "delimiters" :-)
>
> In fairness to this Texas boy, this is not a comment I wrote, it just looks
> like an 'add' because of the way the diff worked out. (See a couple of lines
> down)
*nod* Yup, sorry.
>> This assignment-in-condition construction got me a warning:
>>
>> subversion/libsvn_subr/opt.c: In function svn_opt_args_to_target_array3:
>> subversion/libsvn_subr/opt.c:1010: warning: suggest parentheses
>> around assignment used as truth value
>
> I have trouble seeing any warnings in the extremely verbose Subversion output,
> if it could build more like, say, the Linux kernel it'd be much easier :).
> Sure I could use 'make -s', but then it looks like it is hung! I like the
> warm fuzzy of status text scrolling by, but not TOO much.
Sure, if you're trying to detect them with unassisted human eyes,
they're hard to see. But if you build inside an IDE or some other of
environment that detects the "warning:" and "error:" lines for you, then
it's no problem. Or if you just manually run a search tool like grep
over the output, it's no problem either.
There are many build environments available. The one I happen to use is
Emacs's compilation mode, but that's a matter of taste. The main thing
is that you needn't (and shouldn't) depend on vgrep to find warnings
(ahem, http://en.wikipedia.org/wiki/Vgrep#Humorous_terminology).
>> Maybe it would be better to just have svn_opt__arg_canonicalize_path()
>> take a new *peg_rev parameter, which (if the parameter itself is
>> non-NULL) returns any peg rev found, as well as returning the
>> canonicalized true target. Then we'd have the logic for parsing peg
>> revs centralized in one place (including handling escaping), and some
>> duplicate code could go away.
>>
>> Was this discussed already? I didn't follow the thread in detail.
>
> Only between the voices in my head. I thought about doing something like this
> but decided against it for two reasons:
> - Returning a peg revision seperately does not really do what the function
> name implies. The function 'canonicalizes' the given value, it
> just happens to also be able to cope with peg revisions. Returning
> a peg revision would be like a mix of a 'canonicalize' function and my
> split_arg_at_peg_revision() function.
Sure, but remember it's fine to change the name of an internal function
(that is, any file-static function, and any of the two-underscore
functions like "svn_foo__bar_baz_qux()").
> - If I did go that route I knew that I would need to implement the 'if peg
> rev argument is NULL don't return one there' and thought the devs
> did not like that kind of thing in one of my previous patches.
Hmm, I don't know who said what, but we do that in other places, and I
don't recall any objections to the practice. I think it's fine.
> I could create a helper function that took a target and
> determined whether it is a adm dir, but could recognoize peg revisions. Not
> sure what I would call such a function at the moment, but it is an idea.
The term "adm dir" usually means a .svn/ subdirectory in a working
copy. Is that what you meant here?
> When I initially coded this I did not like the duplication that much either,
> but decided to keep it because I really felt like the
> svn_client_args_to_target_array() and svn_opt_args_to_target_array3()
> already had so much in common that this fit right in. I couldn't create a
> private helper function like I did with split_arg_at_peg_revision() because
> they are in two different libraries.
I think this is what the subversion/include/private/ section of the code
is for -- it's okay to have non-public but cross-library functions in
Subversion.
-Karl
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-05-13 22:10:46 CEST