On 1/2/11 12:06 PM, Daniel Shahaf wrote:
> Blair Zajac wrote on Sun, Jan 02, 2011 at 11:51:45 -0800:
>> On 1/1/2011 11:19 AM, danielsh_at_apache.org wrote:
>>> Author: danielsh
>>> Date: Sat Jan 1 19:19:53 2011
>>> New Revision: 1054273
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1054273&view=rev
>>> Log:
>>> Avoid hard-coding a line number in a C test.
>>>
>>> * subversion/include/svn_error_codes.h
>>> (SVN_ERR_ASSERTION_ONLY_TRACING_LINKS): New error code.
>>>
>>> * subversion/libsvn_subr/error.c
>>> (svn_error_purge_tracing):
>>> Return a specific error code, rather than a specific line number :-).
>>>
>>> * subversion/tests/libsvn_subr/error-test.c
>>> (test_error_purge_tracing):
>>> Update expectations. While here, fix a bug where ERR3_COPY would
>>> potentially be uninitialized.
>>
>> There isn't a bug here. err3_copy would only be used if err3 is not
>> NULL, which is enforced by the SVN_TEST_ASSERT(err3) before err3_copy is
>> used. So this code can be removed from the test:
>>
>> else
>> err3_copy.apr_err = APR_SUCCESS;
>>
>
> Ah, I see. You are correct, of course, the standing code was correct.
>
> But, I think it's better not to rely on that; someone could refactor the
> code tomorrow and leave err3_copy uninitialized.
The code doesn't make any assumptions about the return from
svn_error_purge_tracing() and as it asserts various conditions (such as the
input error and output error having different pools), and as it discovers what
is returned, it than can clear the errors. The errors are cleared as soon as it
is safe, so I don't think the ordering can change much.
> Could we just move the SVN_TEST_ASSERT(err3) further up, and remove the
> "if (err3)" entirely?
If the SVN_TEST_ASSERT(err3) is moved up, then it will happen before
svn_error_clear(err), which if the assertion fails, leads to an error leak.
Blair
Received on 2011-01-02 21:22:06 CET