You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Hudson <gh...@MIT.EDU> on 2003/11/30 16:42:03 UTC
Re: [PATCH] extend svn_io_file_* with calls for read(_full) and
write(_full)
On Sun, 2003-11-30 at 04:08, Erik Huelsmann wrote:
> I was about to commit this change when Greg Hudson said he would not have
> coded it this way, so I'm submitting for review.
Er, I think what Erik meant is: he wrote something, I said it didn't
look right to me, we talked about it and came up with something new, but
based on that he wants review on the finished product.
Three comments:
> +/** Wrapper for @c apr_file_read(), which see. */
I guess by "which see," you mean, "see the documentation for that
function," but I don't think that's very clear.
> if (fname)
> - SVN_ERR (svn_path_cstring_to_utf8 (fname_utf8, fname, pool));
> - else
> - *fname_utf8 = NULL;
> + {
> + svn_error_t *err;
>
> + err = svn_path_cstring_to_utf8 (&fname, fname, pool);
> + if (err)
> + return err;
> + }
> + *fname_utf8 = fname;
> return SVN_NO_ERROR;
I guess the point here is to guarantee that *fname_utf8 doesn't get
garbage of UTF-8 conversion fails. But you also wrote out an SVN_ERR()
the long way; SVN_ERR(svn_path_cstring_to_utf8 (&fname, fname, pool))
should do fine.
> if (status)
> - return svn_error_createf (status, NULL,
> - "svn_io_file_open: can't open '%s'", fname);
> + {
> + apr_strerror (status, errbuf, sizeof(errbuf));
> + SVN_ERR (svn_utf_cstring_to_utf8 (&err_str, errbuf, pool));
> + return svn_error_createf
> + (status, NULL, "Can't open file '%s': %s", fname, err_str);
> + }
The behavior on error is different from that of the IO wrapper. But
then, UTF-8 conversion of an APR error string is never going to fail, so
perhaps this isn't worth worrying about.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] extend svn_io_file_* with calls for read(_full) and write(_full)
Posted by Erik Huelsmann <e....@gmx.net>.
[ snip: yes I meant that ]
> Three comments:
> > +/** Wrapper for @c apr_file_read(), which see. */
>
> I guess by "which see," you mean, "see the documentation for that
> function," but I don't think that's very clear.
Ok; it's the way some other functions in that header are documented too, but
I can fix all of them.
[snip]
> > + svn_error_t *err;
> >
> > + err = svn_path_cstring_to_utf8 (&fname, fname, pool);
> > + if (err)
> > + return err;
> > + }
> > + *fname_utf8 = fname;
> > return SVN_NO_ERROR;
>
> I guess the point here is to guarantee that *fname_utf8 doesn't get
> garbage of UTF-8 conversion fails. But you also wrote out an SVN_ERR()
> the long way; SVN_ERR(svn_path_cstring_to_utf8 (&fname, fname, pool))
> should do fine.
That is indeed the point, but the if(err) return err is an ooops. Good I
submitted for review...
> > if (status)
> > - return svn_error_createf (status, NULL,
> > - "svn_io_file_open: can't open '%s'",
> fname);
> > + {
> > + apr_strerror (status, errbuf, sizeof(errbuf));
> > + SVN_ERR (svn_utf_cstring_to_utf8 (&err_str, errbuf, pool));
> > + return svn_error_createf
> > + (status, NULL, "Can't open file '%s': %s", fname, err_str);
> > + }
>
> The behavior on error is different from that of the IO wrapper. But
> then, UTF-8 conversion of an APR error string is never going to fail, so
> perhaps this isn't worth worrying about.
I'll have a look at it and see if it needs lots of code to fix this.
Fixed commit comming up. Thanks for the review.
bye,
Erik.
--
Neu bei GMX: Preissenkung f�r MMS-Versand und FreeMMS!
Ideal f�r alle, die gerne MMS verschicken:
25 FreeMMS/Monat mit GMX TopMail.
http://www.gmx.net/de/cgi/produktemail
+++ GMX - die erste Adresse f�r Mail, Message, More! +++
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] extend svn_io_file_* with calls for read(_full) and write(_full)
Posted by Julian Foad <ju...@btopenworld.com>.
Greg Hudson wrote:
> On Sun, 2003-11-30 at 04:08, Erik Huelsmann wrote:
>
>>I was about to commit this change when Greg Hudson said he would not have
>>coded it this way, so I'm submitting for review.
>
> Er, I think what Erik meant is: he wrote something, I said it didn't
> look right to me, we talked about it and came up with something new, but
> based on that he wants review on the finished product.
>
> Three comments:
>
>>+/** Wrapper for @c apr_file_read(), which see. */
>
> I guess by "which see," you mean, "see the documentation for that
> function," but I don't think that's very clear.
I think Eric was just copying this from all the existing functions which somebody else wrote. I do vaguely recall someone on the mailing list a few months ago having trouble understanding this construct, so maybe we should change the wording in every case, but not just within Erik's new functions.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] extend svn_io_file_* with calls for read(_full) and write(_full)
Posted by Branko Čibej <br...@xbc.nu>.
mark benedetto king wrote:
>On Sun, Nov 30, 2003 at 11:42:03AM -0500, Greg Hudson wrote:
>
>
>>On Sun, 2003-11-30 at 04:08, Erik Huelsmann wrote:
>>
>>
>>>I was about to commit this change when Greg Hudson said he would not have
>>>coded it this way, so I'm submitting for review.
>>>
>>>
>>Er, I think what Erik meant is: he wrote something, I said it didn't
>>look right to me, we talked about it and came up with something new, but
>>based on that he wants review on the finished product.
>>
>>Three comments:
>>
>>
>>
>>>+/** Wrapper for @c apr_file_read(), which see. */
>>>
>>>
>>I guess by "which see," you mean, "see the documentation for that
>>function," but I don't think that's very clear.
>>
>>
>>
>
>There are a bunch of comments like this running around. I agree, though,
>that this construction seems a bit awkward, especially since Doxygen will
>make the reference into a link.
>
>
Huh, that's perfectly clear and gramatically correct English.
(Whilst it may not be American, we /are/ an international community... :-)
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] extend svn_io_file_* with calls for read(_full) and write(_full)
Posted by mark benedetto king <mb...@lowlatency.com>.
On Sun, Nov 30, 2003 at 11:42:03AM -0500, Greg Hudson wrote:
> On Sun, 2003-11-30 at 04:08, Erik Huelsmann wrote:
> > I was about to commit this change when Greg Hudson said he would not have
> > coded it this way, so I'm submitting for review.
>
> Er, I think what Erik meant is: he wrote something, I said it didn't
> look right to me, we talked about it and came up with something new, but
> based on that he wants review on the finished product.
>
> Three comments:
>
> > +/** Wrapper for @c apr_file_read(), which see. */
>
> I guess by "which see," you mean, "see the documentation for that
> function," but I don't think that's very clear.
>
There are a bunch of comments like this running around. I agree, though,
that this construction seems a bit awkward, especially since Doxygen will
make the reference into a link.
--ben
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org