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

Re: svn commit: r38193 - trunk/subversion/libsvn_subr

From: Stefan Sperling <stsp_at_elego.de>
Date: Thu, 25 Jun 2009 15:29:41 +0100

On Thu, Jun 25, 2009 at 03:05:01PM +0100, Julian Foad wrote:
> Stefan Sperling wrote:
> > On Thu, Jun 25, 2009 at 02:55:15PM +0200, Greg Stein wrote:
> > > Shouldn't those arguments be 'const' ?
> >
> > Yeah they should be.
> > Don't blame me, I didn't write that code, I just moved it :P
> >
> > I'll consider changing the function's parameter anyway as
> > outlined in the log message (to const char *buf and apr_size_t len).
>
> Would there be an advantage in taking a length parameter instead of an
> end pointer? I think you would still need the equivalent assertion
> (length >= 0), because C doesn't protect against passing a negative
> number even if the data type is "unsigned".

But that's a good reason for consistenly using unsinged.
On all levels. If the function already wants unsinged, the caller
should try to pass unsigned, too, without casting, anywhere.

> If you want to use a length
> for other reasons, such as consistency with other APIs, that's fine of
> course.

It's just consistency, really. And also because an API that allows you
to simply pass sizeof(buf) instead of pointer offsets is much easier
to use, IMO. No brain-power involved for the programmer, because
the programmer can assume that the function will take care not to
overrun the buffer. That's just a pattern I've grown used to.

> Also:
>
> > /* Return the first eol marker found in [@a buf, @a endp) as a
> > * NUL-terminated string, or NULL if no eol marker is found.
> > *
> > * If the last valid character of @a buf is the first byte of a
> > * potentially two-byte eol sequence, just return "\r", that is,
>
> Shouldn't it return what's there (which could be "\n" or "\r") rather
> than always "\r" ...
>
> > * assume @a buf represents a CR-only file. This is correct for callers
>
> ... and "CR-only or LF-only"?

I don't know. The function has always been doing this, even
while it was still a static function in libsvn_diff.
I don't want to change this behaviour because I'd have to tweak
the callers, too.

Stefan
Received on 2009-06-25 16:30:20 CEST

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