[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] extend svn_subst_translate_string() to record whether re-encoding and/or line ending translation were performed (v. 3)

From: Julian Foad <julian.foad_at_wandisco.com>
Date: Mon, 15 Nov 2010 12:50:03 +0000

On Sat, 2010-11-13, Daniel Trebbien wrote:
> > The only difference between translate_newline() and translate_newline_alt()
> > is the lines of code from here to the end of the function. I think you
> > could keep translate_newline() as is, and just move these checks elsewhere
> > (to a new function or to the caller; I haven't thought about this in
> > detail).
> >
> > In any case, please avoid the code duplication if possible. (Function
> > pointers are nice, but sometimes just a way to shoot yourself in the
> > foot.) If you're unsure what approach to take, do brainstorm on-list
> > before sending a new iteration of the patch. (That saves time to you
> > and to us)
>
> After thinking about this problem some more, I thought about using a
> macro solution. [...]
>
> Do you like this idea? I have attached C code that I have tested. In
> this sample, the macro that outputs the definitions of
> translate_newline() and translate_newline_alt() is called
> DEFINE_TRANSLATE_NEWLINE_FN.

Hi Daniel T. Sorry to jump in here just to say this, but: no, please
don't define the function inside a macro. I'm not sure how to concisely
explain the reasons why, but I'd be happy to try if you need me to,
although I'm sure you can basically see or "feel" why. And also let's
not have one function overwrite a pointer to itself with a pointer to
another function. I don't think the problem we are facing needs such a
complex solution.

Let's step back. Someone mentioned a concern about efficiency of
checking something once per newline. What's the check? Just "if
((newline_len != eol_str_len) || (*newline_buf != *eol_str))"? If so,
that's nothing to worry about. Or if there's a more expensive check,
you could prefix it with "if (! translated_eol)". That's the most
complexity it needs, I think!

As for Daniel S's subpool question - my guess is that he's right in
thinking that creating and destroying a subpool is not worthwhile.

- Julian
Received on 2010-11-15 13:50:48 CET

This is an archived mail posted to the Subversion Dev mailing list.