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 2004/05/14 00:53:16 UTC
Re: svn commit: r9697 - in trunk/subversion: clients/cmdline
include libsvn_client libsvn_subr
On Wed, 2004-05-12 at 18:57, brane@tigris.org wrote:
> + svn_error_t *err = SVN_NO_ERROR;
> + int i;
> +
> + for (i = 0; checklist[i].label != NULL; ++i)
> + {
> + const svn_version_t *lib_version = checklist[i].version_query();
> + if (!svn_ver_compatible (my_version, lib_version))
> + err = mismatch_error (checklist[i].label, lib_version, err);
> + }
> +
> + return err;
This could leak errors. Hardly a big deal in practice, but adding "&&
!err" to the loop condition, or a break to the body of the if statement,
would make the code more correct.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r9697 - in trunk/subversion: clients/cmdline include
libsvn_client libsvn_subr
Posted by Branko Čibej <br...@xbc.nu>.
Greg Hudson wrote:
>On Thu, 2004-05-13 at 21:00, Branko Čibej wrote:
>
>
>>No, it would make the code wrong IMHO because it would stop at the first
>>incompatibility instead of listing them all.
>>
>>
>
>Sorry, I missed that you were chaining the errors together. I continue
>to believe that it's a mistake to chain together unrelated errors
>(nested errors should have an inner-to-outer relationship, not a simple
>concatenation relationship), but we do so in other parts of the code
>already.
>
>
I've noticed this problem myself, and I think the long-term solution is
to have two-dimensional chaining -- i.e., each error can have both
siblings and children. I think that would cover all the possibilities.
>On the other hand, this error-generator callback strikes me as horribly
>over-engineered. We have no other functions which use callbacks to
>generate error messages. If the caller needs to add information to the
>error message generated by the library routine, it can do so using error
>chaining (of the more proper variety).
>
>
Yes, I think I agree... I'll remove that generator function. Simpler
life, etc. :-)
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r9697 - in trunk/subversion:
clients/cmdline include libsvn_client libsvn_subr
Posted by Greg Hudson <gh...@MIT.EDU>.
On Thu, 2004-05-13 at 21:00, Branko Čibej wrote:
> No, it would make the code wrong IMHO because it would stop at the first
> incompatibility instead of listing them all.
Sorry, I missed that you were chaining the errors together. I continue
to believe that it's a mistake to chain together unrelated errors
(nested errors should have an inner-to-outer relationship, not a simple
concatenation relationship), but we do so in other parts of the code
already.
On the other hand, this error-generator callback strikes me as horribly
over-engineered. We have no other functions which use callbacks to
generate error messages. If the caller needs to add information to the
error message generated by the library routine, it can do so using error
chaining (of the more proper variety).
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r9697 - in trunk/subversion: clients/cmdline include
libsvn_client libsvn_subr
Posted by Branko Čibej <br...@xbc.nu>.
Branko Čibej wrote:
> Greg Hudson wrote:
>
>> On Wed, 2004-05-12 at 18:57, brane@tigris.org wrote:
>>
>>
>>> + svn_error_t *err = SVN_NO_ERROR;
>>> + int i;
>>> +
>>> + for (i = 0; checklist[i].label != NULL; ++i)
>>> + {
>>> + const svn_version_t *lib_version = checklist[i].version_query();
>>> + if (!svn_ver_compatible (my_version, lib_version))
>>> + err = mismatch_error (checklist[i].label, lib_version, err);
>>> + }
>>> +
>>> + return err;
>>>
>>
>>
>> This could leak errors. Hardly a big deal in practice, but adding "&&
>> !err" to the loop condition, or a break to the body of the if statement,
>> would make the code more correct.
>>
>>
> No, it would make the code wrong IMHO because it would stop at the
> first incompatibility instead of listing them all.
>
> How about this: I remove the 'child' parameter from
> svn_ver_error_generator_t and set it explicitly in this loop, so that
> we don't rely on the mismatch_error implementation being correct.
That, or I change the mismatch_error type so that it only creates the
error string, and handle the error creation and chaining here. Even safer.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn commit: r9697 - in trunk/subversion: clients/cmdline include
libsvn_client libsvn_subr
Posted by Branko Čibej <br...@xbc.nu>.
Greg Hudson wrote:
>On Wed, 2004-05-12 at 18:57, brane@tigris.org wrote:
>
>
>>+ svn_error_t *err = SVN_NO_ERROR;
>>+ int i;
>>+
>>+ for (i = 0; checklist[i].label != NULL; ++i)
>>+ {
>>+ const svn_version_t *lib_version = checklist[i].version_query();
>>+ if (!svn_ver_compatible (my_version, lib_version))
>>+ err = mismatch_error (checklist[i].label, lib_version, err);
>>+ }
>>+
>>+ return err;
>>
>>
>
>This could leak errors. Hardly a big deal in practice, but adding "&&
>!err" to the loop condition, or a break to the body of the if statement,
>would make the code more correct.
>
>
No, it would make the code wrong IMHO because it would stop at the first
incompatibility instead of listing them all.
How about this: I remove the 'child' parameter from
svn_ver_error_generator_t and set it explicitly in this loop, so that we
don't rely on the mismatch_error implementation being correct.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org