You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Blair Zajac <bl...@orcaware.com> on 2010/12/24 06:10:50 UTC
Safe to add a field to svn_error_t?
The addition of "tracing" to svn_error_t in 1.7 currently uses a special message
string "traced call". Instead of doing this, can we safely add an svn_boolean_t
to svn_error_t the indicates if it is a traced error?
While svn_error_t is public, I'm hoping most people are creating them with
svn_error_create*(), so if that's the case, then code written and compiled
against svn <= 1.6 would work fine when linked against a 1.7 library.
Thoughts?
Blair
Re: Safe to add a field to svn_error_t?
Posted by Stefan Fuhrmann <eq...@web.de>.
On 24.12.2010 19:32, Blair Zajac wrote:
> On 12/24/10 6:07 AM, Hyrum K Wright wrote:
>> On Fri, Dec 24, 2010 at 12:10 AM, Blair Zajac<bl...@orcaware.com>
>> wrote:
>>> The addition of "tracing" to svn_error_t in 1.7 currently uses a
>>> special
>>> message string "traced call". Instead of doing this, can we safely
>>> add an
>>> svn_boolean_t to svn_error_t the indicates if it is a traced error?
>>>
>>> While svn_error_t is public, I'm hoping most people are creating
>>> them with
>>> svn_error_create*(), so if that's the case, then code written and
>>> compiled
>>> against svn<= 1.6 would work fine when linked against a 1.7 library.
>>>
>>> Thoughts?
>>
>> We don't explicitly say "only use approved methods to create this
>> object", but we are pretty implicit about it, with an entire
>> documentation group for "Error creation and destruction". I'm on the
>> fence on this, but would lean toward "go ahead and extend the struct"
>> if pressured.
>>
>> What problem are you trying to solve?
>
> There's this comment in svn_error__is_tracing_link():
>
> svn_boolean_t
> svn_error__is_tracing_link(svn_error_t *err)
> {
> #ifdef SVN_ERR__TRACING
> /* ### A strcmp()? Really? I think it's the best we can do unless
> ### we add a boolean field to svn_error_t that's set only for
> ### these "placeholder error chain" items. Not such a bad idea,
> ### really... */
> return (err && err->message && !strcmp(err->message, SVN_ERR__TRACED));
> #else
> return FALSE;
> #endif
> }
What is the use-case for this function anyways?
IMHO, an svn_error_t says "There was problem
$message when executing the instruction at $file,
$line. To be more precise it was <inner error info
follows>."
The fact that most error messages are just "traced
call" simply means we did not bother to give more
specific descriptions. But I don't see a real semantic
difference because everybody along the calling
chain may have contributed to the problem.
At best one might use this function to reduce the
length of the error report hoping that all the default
text in between are not relevant to the particular
scenario.
>
> I thought of two other solutions:
>
> 1) a sentinel message and we compare by address.
> 2) negative source file line numbers.
>
To me, they seems at least as "hacky" as the
current solution. I could easily live with them but
so can I with the current code.
-- Stefan^2.
Re: Safe to add a field to svn_error_t?
Posted by Blair Zajac <bl...@orcaware.com>.
On 12/24/10 10:32 AM, Blair Zajac wrote:
> On 12/24/10 6:07 AM, Hyrum K Wright wrote:
>> On Fri, Dec 24, 2010 at 12:10 AM, Blair Zajac<bl...@orcaware.com> wrote:
>>> The addition of "tracing" to svn_error_t in 1.7 currently uses a special
>>> message string "traced call". Instead of doing this, can we safely add an
>>> svn_boolean_t to svn_error_t the indicates if it is a traced error?
>>>
>>> While svn_error_t is public, I'm hoping most people are creating them with
>>> svn_error_create*(), so if that's the case, then code written and compiled
>>> against svn<= 1.6 would work fine when linked against a 1.7 library.
>>>
>>> Thoughts?
>>
>> We don't explicitly say "only use approved methods to create this
>> object", but we are pretty implicit about it, with an entire
>> documentation group for "Error creation and destruction". I'm on the
>> fence on this, but would lean toward "go ahead and extend the struct"
>> if pressured.
>>
>> What problem are you trying to solve?
>
> There's this comment in svn_error__is_tracing_link():
>
> svn_boolean_t
> svn_error__is_tracing_link(svn_error_t *err)
> {
> #ifdef SVN_ERR__TRACING
> /* ### A strcmp()? Really? I think it's the best we can do unless
> ### we add a boolean field to svn_error_t that's set only for
> ### these "placeholder error chain" items. Not such a bad idea,
> ### really... */
> return (err && err->message && !strcmp(err->message, SVN_ERR__TRACED));
> #else
> return FALSE;
> #endif
> }
>
>
> I thought of two other solutions:
>
> 1) a sentinel message and we compare by address.
Realized later that this won't work because svn_error_dup() will copy the
message and constructing a new error also copies the message, so the comparing
addresses won't work.
Blair
Re: Safe to add a field to svn_error_t?
Posted by Blair Zajac <bl...@orcaware.com>.
On 12/24/10 6:07 AM, Hyrum K Wright wrote:
> On Fri, Dec 24, 2010 at 12:10 AM, Blair Zajac<bl...@orcaware.com> wrote:
>> The addition of "tracing" to svn_error_t in 1.7 currently uses a special
>> message string "traced call". Instead of doing this, can we safely add an
>> svn_boolean_t to svn_error_t the indicates if it is a traced error?
>>
>> While svn_error_t is public, I'm hoping most people are creating them with
>> svn_error_create*(), so if that's the case, then code written and compiled
>> against svn<= 1.6 would work fine when linked against a 1.7 library.
>>
>> Thoughts?
>
> We don't explicitly say "only use approved methods to create this
> object", but we are pretty implicit about it, with an entire
> documentation group for "Error creation and destruction". I'm on the
> fence on this, but would lean toward "go ahead and extend the struct"
> if pressured.
>
> What problem are you trying to solve?
There's this comment in svn_error__is_tracing_link():
svn_boolean_t
svn_error__is_tracing_link(svn_error_t *err)
{
#ifdef SVN_ERR__TRACING
/* ### A strcmp()? Really? I think it's the best we can do unless
### we add a boolean field to svn_error_t that's set only for
### these "placeholder error chain" items. Not such a bad idea,
### really... */
return (err && err->message && !strcmp(err->message, SVN_ERR__TRACED));
#else
return FALSE;
#endif
}
I thought of two other solutions:
1) a sentinel message and we compare by address.
2) negative source file line numbers.
Blair
Re: Safe to add a field to svn_error_t?
Posted by Hyrum K Wright <hy...@hyrumwright.org>.
On Fri, Dec 24, 2010 at 12:10 AM, Blair Zajac <bl...@orcaware.com> wrote:
> The addition of "tracing" to svn_error_t in 1.7 currently uses a special
> message string "traced call". Instead of doing this, can we safely add an
> svn_boolean_t to svn_error_t the indicates if it is a traced error?
>
> While svn_error_t is public, I'm hoping most people are creating them with
> svn_error_create*(), so if that's the case, then code written and compiled
> against svn <= 1.6 would work fine when linked against a 1.7 library.
>
> Thoughts?
We don't explicitly say "only use approved methods to create this
object", but we are pretty implicit about it, with an entire
documentation group for "Error creation and destruction". I'm on the
fence on this, but would lean toward "go ahead and extend the struct"
if pressured.
What problem are you trying to solve?
-Hyrum