Re: [PATCH] svn_wc_ventry(_ex) to eliminate heavily repeated code

On Mon, 19 Mar 2007, Erik Huelsmann wrote:

> On 3/19/07, Ivan Zhakov <chemodax@gmail.com> wrote:
> >On 3/17/07, Erik Huelsmann <ehuels@gmail.com> wrote:
> >> In libsvn_wc, we have a number of patterns which are extremely common,
> >> yet verbosely repeated every time. One of them is addressed in the
> >> patch attached.
> >>
> >> The pattern is:
> >>
> >> - get an entry using svn_wc_entry()
> >> - check for a non-NULL entry and,
> >> return a not-versioned error if the entry *is* NULL
> >>
> >> The only thing I could figure out why this would be is that we want to
> >> have file and line numbers in the error message, so, I introduced a
> >> macro and a new function.
> >>
> >> The function svn_wc_ventry_ex (ventry as for 'valid entry') which
> >> takes a file name and a line number. The macro svn_wc_ventry()
> >> forwards to the function, adding the filename and lineno.
> >>
> >> I know it isn't our usual pattern, but it's one of the ways I see to
> >> make libsvn_wc a bit more readable.
> >>
> >> BTW: This patch has 2 failing tests, because of changed error output.
> >> No real problems, as far as I can tell.
> >>
> >> Comments?
> >>
> >I like this idea, but I prefer to add this helper locally to libsvn_wc
> >and libsvn_client. Because exporting and maintaining such helpers
> >isn't good way. Interface of library should be minimal as possible. So
> >I propose to create svn_wc__ventry_ex() and
> >svn__clinet_wc_ventry_ex().
> >Yes this introduce *some* code duplication, but I think it's better
> >than exporting helpers from our library.
>
> But we already do in the context of error handling: see svn_error__locate().
>
> I also wouldn't like to export helpers, but I think it's 'madness' to
> start maintaining code in several layers separately. What if we were
> to change error creation to move away from global variables and use
> the same scheme as the one here?
>
> Next to that, I tink that documenting something as 'internal' should
> be enough for people not to use it... It's not like they have no other
> option.
>
> You are aware that all svn_*__ symbols are exported from the libraries
> (at least on Linux) too?

As Vlad suggested on IRC, we now have the include/private/ area. Paul
and I recently created a svn_wc_private.h in there for a
project-private API that we don't want exported by our public header
files.

Our svn_*__* APIs can start following this pattern. HACKING should be
updated to recommend its usage.