You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Malcolm Rowe <ma...@farside.org.uk> on 2007/09/24 09:20:19 UTC
Re: svn commit: r26755 - trunk/subversion/libsvn_subr
On Sat, Sep 22, 2007 at 01:43:40PM -0700, dionisos@tigris.org wrote:
> Eliminate endless loop edge-case (resolves issue #2577).
>
> Before, if no character could be converted, but the remaining buffer size
> was 3 or more, we'd loop endlessly.
I must admit that I'm unsure as to how this is possible - do you have an
example case? (We only convert from UTF-8 <> native, and I don't believe
there are any native encodings that include characters outside the
Unicode BMP, or that take more than three [actually, two] bytes per
character to encode natively).
> - } while (! apr_err && srclen);
> + } while (apr_err == APR_SUCCESS && srclen != 0);
[Is the latter really better than the former? APR_SUCCESS is zero by
definition.]
Regards,
Malcolm
Re: svn commit: r26755 - trunk/subversion/libsvn_subr
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 9/26/07, Greg Hudson <gh...@mit.edu> wrote:
> On Mon, 2007-09-24 at 13:24 +0100, Malcolm Rowe wrote:
> > Sure, though I had got the impression that the current guidance was to
> > avoid APR_SUCCESS completely because it was deprecated or similar.
> > However, I can't find any reference to that, so I've probably
> > mis-remembered.
>
> You may be thinking of the APR_IS_SUCCESS macro?
>
> (I have no idea why apr would like to regain the freedom to redefine
> APR_SUCCESS. That's an odd choice. But "status == APR_SUCCESS" is
> clearer than "!status" so there's some value there.)
APR explicitly does NOT want the freedom to redefine APR_SUCCESS, it's
defined as zero for a reason, and will stay that way. !status (or
!error if you pick your variable name for readability) is perfectly
valid when testing APR errors.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r26755 - trunk/subversion/libsvn_subr
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2007-09-24 at 13:24 +0100, Malcolm Rowe wrote:
> Sure, though I had got the impression that the current guidance was to
> avoid APR_SUCCESS completely because it was deprecated or similar.
> However, I can't find any reference to that, so I've probably
> mis-remembered.
You may be thinking of the APR_IS_SUCCESS macro?
(I have no idea why apr would like to regain the freedom to redefine
APR_SUCCESS. That's an odd choice. But "status == APR_SUCCESS" is
clearer than "!status" so there's some value there.)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r26755 - trunk/subversion/libsvn_subr
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Sep 24, 2007 at 12:05:55PM +0200, Erik Huelsmann wrote:
> > I must admit that I'm unsure as to how this is possible - do you have an
> > example case? (We only convert from UTF-8 <> native, and I don't believe
> > there are any native encodings that include characters outside the
> > Unicode BMP, or that take more than three [actually, two] bytes per
> > character to encode natively).
>
> Not currently, but we also use a special svn_revnum_t - and not an int
> - 'just in case'; with which I want to say: it's often hard to find
> bugs resulting from hardcoded limits, especially when they're
> implicit.
>
Fair enough. I was just wondering whether you had an example to
demonstrate the problem, since it appeared like the case you were fixing
was 'impossible'. (And I agree that something being 'impossible' is no
reason to avoid fixing it, especially if you can simplify the code :-)).
> > > - } while (! apr_err && srclen);
> > > + } while (apr_err == APR_SUCCESS && srclen != 0);
> >
> > [Is the latter really better than the former? APR_SUCCESS is zero by
> > definition.]
>
> Yes, that's true, but using APR_SUCCESS means that we stay source
> compatible with APR 2.0 and higher, where they're free to redefine it
> to any other value.
>
Sure, though I had got the impression that the current guidance was to
avoid APR_SUCCESS completely because it was deprecated or similar.
However, I can't find any reference to that, so I've probably
mis-remembered.
Regards,
Malcolm
Re: svn commit: r26755 - trunk/subversion/libsvn_subr
Posted by Erik Huelsmann <eh...@gmail.com>.
On 9/24/07, Malcolm Rowe <ma...@farside.org.uk> wrote:
> On Sat, Sep 22, 2007 at 01:43:40PM -0700, dionisos@tigris.org wrote:
> > Eliminate endless loop edge-case (resolves issue #2577).
> >
> > Before, if no character could be converted, but the remaining buffer size
> > was 3 or more, we'd loop endlessly.
Right, but the theoretical limit (currently) is 4. And since I had to
change the loop anyway (to make it fit the theoretical limit), why
hardcode a limit at all? This shifts the problem
> I must admit that I'm unsure as to how this is possible - do you have an
> example case? (We only convert from UTF-8 <> native, and I don't believe
> there are any native encodings that include characters outside the
> Unicode BMP, or that take more than three [actually, two] bytes per
> character to encode natively).
Not currently, but we also use a special svn_revnum_t - and not an int
- 'just in case'; with which I want to say: it's often hard to find
bugs resulting from hardcoded limits, especially when they're
implicit.
The other option I considered is to define
SVN_UTF__MAX_CHARACTER_OCTETS in the header, which would make it
easier to trace where we're using the maximum character assumption.
That would however require me to trace all assumptions (otherwise I
would be suggesting that this loop is the only place where we have
this assumption). Not having assumptions looked better to me.
> > - } while (! apr_err && srclen);
> > + } while (apr_err == APR_SUCCESS && srclen != 0);
>
> [Is the latter really better than the former? APR_SUCCESS is zero by
> definition.]
Yes, that's true, but using APR_SUCCESS means that we stay source
compatible with APR 2.0 and higher, where they're free to redefine it
to any other value.
> Regards,
> Malcolm
bye,
Erik.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org