On Mon, Oct 28, 2002 at 06:21:51PM -0500, Greg Hudson wrote:
>...
> There are a few opportunities for exception-handling cleanups:
> * svn_error_clear_all() is dangerous, and must be replaced with an
> svn_error_clear().
Yup.
> * We should replace svn_error_locate() with extra arguments to
> svn_create_error()'s internal name.
> * We should always collect line and file information, but only
> expose it with the appropriate flag set. This will help with
> debugging and will simplify the internal interface.
Hmm. Did we come to a resolution on the on this? Seems there was still some
open discussion on whether this would excessively bulk up the non-debug
builds.
[ on the rest, I agree with all that you stated, but call out some stuff ]
>...
> ra_dav and ra_local have separate implementations of split_url.
Hmm? I don't see a split_url in ra_dav.
>...
> The ra interface passes dates as strings (in svn_time_to_nts()
> format), for implementation convenience. apr_time_t would make for a
> cleaner interface, though it's more work on the back end.
Yes, this is not good. I think we also use a string in some of the 'svn log'
types of operations.
>...
> Someone added an svn_stream_dup() function to my stream interface.
> Nothing uses it. The only purpose would be to move a stream object
> from one pool to another (it doesn't actually split the stream), and I
> can't imagine why any legitimately designed code would want to do
> that. I think it should go away.
+1 Toss it.
> svn_log_changed_path_t uses a char instead of an enum to specify file
> status. Sloppy.
Yah... never liked that. Some other repos stuff (the nodes stuff, iirc) also
does this.
> Our build system's Makefiles use CFLAGS and LDFLAGS. The GNU Makefile
> standards say:
> If there are C compiler options that *must* be used for proper
> compilation of certain files, do not include them in `CFLAGS'.
> Users expect to be able to specify `CFLAGS' freely themselves.
> We should change gen-make.py to use ALL_CFLAGS for everything except
> optimization options, and then rename EXTRA_CFLAGS to CFLAGS. Same
> deal for CPPFLAGS and LDFLAGS.
This might need an issue to track it well enough. It sounds kind of tricky.
> svn_wc_get_actual_target() sets *target to NULL when anchor is the
> target. That was probably right at the time, but these days we use ""
> to mean an empty relative path, not NULL. As a consequence of this
> function's behavior, a number of contracts are unnecessarily lax,
> including the ra interface's update/switch/status functions.
I recall some part of the RA interface being very relaxed about empty
strings versus NULL points. Something dealing with the authors and dates and
stuff. I certainly have no problem tightening up stuff like that.
>...
> adm_files.c's extend_with_adm_name() doubles as a path concatenation
> function. That seems a little bizarre, but it's pretty entrenched, so
> I'm not likely to do anything about it.
Be nice if there was a way to use svn_path_join_many(). Would probably need
a variant of the join which takes a valist.
>...
> svn_path_basename()'s contract could be to return a pointer into the
> argument string, instead of duplicating the result into a pool
> argument. This would make some code simpler.
I thought about this, but the caller would never know if the passed-in
string is part of the output or not. Today, the caller can change the input
string at will since the contract says the result will always be a new
string in the provided pool. (the doc specifically states it will make a
copy, even when it isn't strictly necessary)
> While building with shared libraries, I ran into a libtool relink
> problem when installing. (Red Hat 7.3.) I've seen this problem
> before, in other code, but forget what the cause was. I should
> investigate this.
Hmm. We definitely relink at install time, but I haven't seen any errors
recently.
>...
> props and wc-props files still have no suffix, so they show up in
> "find". (Arguably, all these files should have prefixes as well as
> suffixes, for added protection.)
Ah. Interesting thought. Prefixes would be interesting. At this rate, we
should just choose a unique name and store that into the 'entries'. It would
save us a good amount of pain with all this prefix/suffix stuff.
[ of course, that would make debugging hard. "what does ekz45931 correspond
to? grr..." ]
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Nov 4 00:21:21 2002