You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Julian Foad <ju...@btopenworld.com> on 2006/01/19 16:10:21 UTC
More error leaks
By putting "__attribute__((warn_unused_result))" on each declaration of a
function that returns "svn_error_t *", I found the following errors being leaked:
subversion/libsvn_fs_fs/fs_fs.c: In function 'svn_fs_fs__set_entry':
subversion/libsvn_fs_fs/fs_fs.c:2773: return value of 'svn_stream_printf'
subversion/libsvn_fs_fs/fs_fs.c:2790: return value of 'svn_stream_printf'
subversion/libsvn_fs_fs/fs_fs.c: In function 'svn_fs_fs__create':
subversion/libsvn_fs_fs/fs_fs.c:4007: return value of 'svn_fs_fs__set_uuid'
--
subversion/libsvn_repos/commit.c: In function 'check_authz':
subversion/libsvn_repos/commit.c:148: return value of function
subversion/libsvn_repos/commit.c: In function 'svn_repos_get_commit_editor4':
subversion/libsvn_repos/commit.c:798: return value of function
--
subversion/libsvn_repos/dump.c: In function 'dump_node':
subversion/libsvn_repos/dump.c:414: return value of 'svn_stream_printf'
--
subversion/libsvn_client/locking_commands.c: In function 'svn_client_lock':
subversion/libsvn_client/locking_commands.c:457: return value of 'svn_wc_adm_close'
subversion/libsvn_client/locking_commands.c: In function 'svn_client_unlock':
subversion/libsvn_client/locking_commands.c:514: return value of 'svn_wc_adm_close'
--
subversion/libsvn_diff/diff_file.c: In function
'svn_diff__file_output_unified_default_hdr':
subversion/libsvn_diff/diff_file.c:894: return value of 'svn_io_stat'
--
subversion/svn/main.c: In function 'main':
subversion/svn/main.c:825: return value of 'svn_cl__help'
subversion/svn/main.c:844: return value of 'svn_cl__help'
subversion/svn/main.c:1147: return value of 'svn_cl__help'
subversion/svn/main.c:1166: return value of 'svn_cl__help'
--
subversion/svn/util.c: In function 'svn_cl__get_log_message':
subversion/svn/util.c:631: return value of 'svn_cmdline_prompt_user'
--
subversion/svnadmin/main.c: In function 'main':
subversion/svnadmin/main.c:1171: return value of 'subcommand_help'
subversion/svnadmin/main.c:1196: return value of 'subcommand_help'
subversion/svnadmin/main.c:1294: return value of 'subcommand_help'
subversion/svnadmin/main.c:1317: return value of 'subcommand_help'
subversion/svnadmin/main.c:1335: return value of 'subcommand_help'
--
subversion/svndumpfilter/main.c: In function 'main':
subversion/svndumpfilter/main.c:1114: return value of 'subcommand_help'
subversion/svndumpfilter/main.c:1137: return value of 'subcommand_help'
subversion/svndumpfilter/main.c:1168: return value of 'subcommand_help'
subversion/svndumpfilter/main.c:1190: return value of 'subcommand_help'
subversion/svndumpfilter/main.c:1209: return value of 'subcommand_help'
--
subversion/svnlook/main.c: In function 'main':
subversion/svnlook/main.c:2020: return value of 'subcommand_help'
subversion/svnlook/main.c:2042: return value of 'subcommand_help'
subversion/svnlook/main.c:2112: return value of 'subcommand_help'
subversion/svnlook/main.c:2142: return value of 'subcommand_help'
subversion/svnlook/main.c:2161: return value of 'subcommand_help'
subversion/svnlook/main.c:2193: return value of 'subcommand_help'
--
subversion/svnsync/main.c: In function 'main':
subversion/svnsync/main.c:1110: return value of 'help'
subversion/svnsync/main.c:1128: return value of 'help'
subversion/svnsync/main.c:1173: return value of 'help'
subversion/svnsync/main.c:1187: return value of 'help'
subversion/svnsync/main.c:1198: return value of 'help'
--
subversion/tests/libsvn_repos/repos-test.c: In function 'commit_editor_authz':
subversion/tests/libsvn_repos/repos-test.c:1564: return value of function
Most of them probably just need "SVN_ERR", but some may be more complex. If
someone is willing to take a look at them and fix all or most of them, that
would be great.
Of course the other question raised is: do we want to consider using such
attributes as a matter of course? Obviously only if it is done neatly and in a
way that doesn't interfere with other compilers; macros can help to achieve
that. I'll have a bit more of a think about that.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Thu, 19 Jan 2006, Julian Foad wrote:
> By putting "__attribute__((warn_unused_result))" on each declaration of a
> function that returns "svn_error_t *", I found the following errors being leaked:
>
...
Nice job, Julian!
> Of course the other question raised is: do we want to consider using such
> attributes as a matter of course? Obviously only if it is done neatly and in a
> way that doesn't interfere with other compilers; macros can help to achieve
> that. I'll have a bit more of a think about that.
>
I think that'd be a great idea! This is a warning that's very useful.
FYI, APR already defines __attribute__ to nothing on non-GCC compilers, so
we could just add the attributes as we've done with printf-like functions.
But we may want to define some macro for it for clarity or
unclumsyfication purposes:-)
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by David James <dj...@collab.net>.
On 1/19/06, Julian Foad <ju...@btopenworld.com> wrote:
> By putting "__attribute__((warn_unused_result))" on each declaration of a
> function that returns "svn_error_t *", I found the following errors being leaked:
>
> subversion/libsvn_fs_fs/fs_fs.c: In function 'svn_fs_fs__set_entry':
> subversion/libsvn_fs_fs/fs_fs.c:2773: return value of 'svn_stream_printf'
> subversion/libsvn_fs_fs/fs_fs.c:2790: return value of 'svn_stream_printf'
> subversion/libsvn_fs_fs/fs_fs.c: In function 'svn_fs_fs__create':
> subversion/libsvn_fs_fs/fs_fs.c:4007: return value of 'svn_fs_fs__set_uuid'
> --
> subversion/libsvn_repos/commit.c: In function 'check_authz':
> subversion/libsvn_repos/commit.c:148: return value of function
> subversion/libsvn_repos/commit.c: In function 'svn_repos_get_commit_editor4':
> subversion/libsvn_repos/commit.c:798: return value of function
> --
> subversion/libsvn_repos/dump.c: In function 'dump_node':
> subversion/libsvn_repos/dump.c:414: return value of 'svn_stream_printf'
> --
> subversion/libsvn_client/locking_commands.c: In function 'svn_client_lock':
> subversion/libsvn_client/locking_commands.c:457: return value of 'svn_wc_adm_close'
> subversion/libsvn_client/locking_commands.c: In function 'svn_client_unlock':
> subversion/libsvn_client/locking_commands.c:514: return value of 'svn_wc_adm_close'
> --
> subversion/libsvn_diff/diff_file.c: In function
> 'svn_diff__file_output_unified_default_hdr':
> subversion/libsvn_diff/diff_file.c:894: return value of 'svn_io_stat'
> --
> subversion/svn/main.c: In function 'main':
> subversion/svn/main.c:825: return value of 'svn_cl__help'
> subversion/svn/main.c:844: return value of 'svn_cl__help'
> subversion/svn/main.c:1147: return value of 'svn_cl__help'
> subversion/svn/main.c:1166: return value of 'svn_cl__help'
> --
> subversion/svn/util.c: In function 'svn_cl__get_log_message':
> subversion/svn/util.c:631: return value of 'svn_cmdline_prompt_user'
> --
> subversion/svnadmin/main.c: In function 'main':
> subversion/svnadmin/main.c:1171: return value of 'subcommand_help'
> subversion/svnadmin/main.c:1196: return value of 'subcommand_help'
> subversion/svnadmin/main.c:1294: return value of 'subcommand_help'
> subversion/svnadmin/main.c:1317: return value of 'subcommand_help'
> subversion/svnadmin/main.c:1335: return value of 'subcommand_help'
> --
> subversion/svndumpfilter/main.c: In function 'main':
> subversion/svndumpfilter/main.c:1114: return value of 'subcommand_help'
> subversion/svndumpfilter/main.c:1137: return value of 'subcommand_help'
> subversion/svndumpfilter/main.c:1168: return value of 'subcommand_help'
> subversion/svndumpfilter/main.c:1190: return value of 'subcommand_help'
> subversion/svndumpfilter/main.c:1209: return value of 'subcommand_help'
> --
> subversion/svnlook/main.c: In function 'main':
> subversion/svnlook/main.c:2020: return value of 'subcommand_help'
> subversion/svnlook/main.c:2042: return value of 'subcommand_help'
> subversion/svnlook/main.c:2112: return value of 'subcommand_help'
> subversion/svnlook/main.c:2142: return value of 'subcommand_help'
> subversion/svnlook/main.c:2161: return value of 'subcommand_help'
> subversion/svnlook/main.c:2193: return value of 'subcommand_help'
> --
> subversion/svnsync/main.c: In function 'main':
> subversion/svnsync/main.c:1110: return value of 'help'
> subversion/svnsync/main.c:1128: return value of 'help'
> subversion/svnsync/main.c:1173: return value of 'help'
> subversion/svnsync/main.c:1187: return value of 'help'
> subversion/svnsync/main.c:1198: return value of 'help'
> --
> subversion/tests/libsvn_repos/repos-test.c: In function 'commit_editor_authz':
> subversion/tests/libsvn_repos/repos-test.c:1564: return value of function
>
>
> Most of them probably just need "SVN_ERR", but some may be more complex. If
> someone is willing to take a look at them and fix all or most of them, that
> would be great.
>
> Of course the other question raised is: do we want to consider using such
> attributes as a matter of course? Obviously only if it is done neatly and in a
> way that doesn't interfere with other compilers; macros can help to achieve
> that. I'll have a bit more of a think about that.
Nice analysis, Julian. I agree with your conclusion. First, we need to
fix places where we ignore errors. Next, if we can find a neat and
clean way to add __attribute__((warn_unused_result)) to declarations
of functions which return errors, that would be great.
Cheers,
David
--
David James -- http://www.cs.toronto.edu/~james
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Marc Sherman <ms...@projectile.ca>.
Julian Foad wrote:
>
> 3. SVN_ERR_FUNC function_name(param one,
> param two);
>
> 4. SVN_ERROR_T *function_name(param one,
> param two);
I know I'm getting well into bike-shed territory here, but can I suggest:
3.5. SVN_ERROR_FN function_name(param one,
param two);
It's closer to svn_error_t than SVN_ERR_FUNC is, so it obscures the
return type slightly less, but it still maintains the tab depth while
including the * in the macro.
- Marc
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Thu, Jan 26, 2006 at 01:00:43PM -0500, John Peacock wrote:
> Julian Foad wrote:
> >Malcolm Rowe wrote:
> >>
> >>By moving
> >>_all_ the callers to use svn_foo2(), don't we lose test coverage of the
> >>original svn_foo()?
> >
> >Yes.
>
> As long as svn_foo() is a wrapper around svn_foo2(), AND during
> development the testing passed with all callers using svn_foo(), is
> there anything reason to require continued testing? I'm thinking
> specifically about the keywords-as-hash patch which was done in multiple
> steps for exactly that reason...
>
If we also introduce svn_foo3() and rewrite svn_foo() to use that directly
(rather than keep svn_foo() forwarding to svn_foo2()), then we don't
get a chance to test svn_foo() again.
Or, more commonly, we can't validate that svn_foo() still works when we
change the implementation of svn_foo2().
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by John Peacock <jp...@rowman.com>.
Julian Foad wrote:
> Malcolm Rowe wrote:
>>
>> By moving
>> _all_ the callers to use svn_foo2(), don't we lose test coverage of the
>> original svn_foo()?
>
> Yes.
As long as svn_foo() is a wrapper around svn_foo2(), AND during
development the testing passed with all callers using svn_foo(), is
there anything reason to require continued testing? I'm thinking
specifically about the keywords-as-hash patch which was done in multiple
steps for exactly that reason...
John
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4501 Forbes Boulevard
Suite H
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5748
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Julian Foad <ju...@btopenworld.com>.
Malcolm Rowe wrote:
>
> By moving
> _all_ the callers to use svn_foo2(), don't we lose test coverage of the
> original svn_foo()?
Yes.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Thu, Jan 26, 2006 at 03:46:11PM +0000, Julian Foad wrote:
> * If there are cases where no use at all is yet being made of a new API,
> there's a fair chance that it won't have been tested at all and won't work
> properly. (That's true for both revised APIs and completely new APIs.)
>
Isn't that also true of deprecated functions though?
That is: assume we have a deprecated function svn_foo() newly
re-implemented in terms of svn_foo2(), as we tend to do. By moving
_all_ the callers to use svn_foo2(), don't we lose test coverage of the
original svn_foo()?
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Julian Foad <ju...@btopenworld.com>.
Greg Hudson wrote:
> On Thu, 2006-01-26 at 02:25 +0000, Julian Foad wrote:
>
>>That's definitely a serious consideration. For a while you made me think this
>>is not worth doing if it is going to cause any such harm. Then I went on to
>>check the usage of our "deprecated" functions and found another whole class of
>>errors that could have been automatically detected. If we don't take the
>>opportunity to use automated checks whenever we can (without too much adverse
>>effect) then we'll suffer.
>
> How, exactly, are we suffering from using our deprecated functions?
> That's just work we've accidentally deferred until 2.0 when we get rid
> of them.
We're not necessarily suffering in any immediate way from using our deprecated
functions - you're right in most cases that it's just deferred work - but:
* In some cases the replacement API provides some improved behaviour which we
believe we are using but in fact we are not using in some part of the program.
For example, svn_wc_process_committed2() recurses via the forwarding function
svn_wc_process_committed() which means that its new "remove_lock" flag is lost
on recursion. It actually seems to achieve the right result, and presumably if
it hadn't this would have been picked up by functional testing, but it is easy
to envisage a similar situation in which some aspect of new behaviour was
missing in certain scenarios that hadn't been tested.
* If there are cases where no use at all is yet being made of a new API,
there's a fair chance that it won't have been tested at all and won't work
properly. (That's true for both revised APIs and completely new APIs.)
* My point was really that we suffer bugs - such as the error leaks that I
found - from not using automated checks that we could easily use. The use of
deprecated functions was just an example of another class of problems that can
be detected and that made me realise the value of such detection; it doesn't
matter whether they can result in actual bugs for that argument to hold.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Greg Hudson <gh...@MIT.EDU>.
On Thu, 2006-01-26 at 02:25 +0000, Julian Foad wrote:
> That's definitely a serious consideration. For a while you made me think this
> is not worth doing if it is going to cause any such harm. Then I went on to
> check the usage of our "deprecated" functions and found another whole class of
> errors that could have been automatically detected. If we don't take the
> opportunity to use automated checks whenever we can (without too much adverse
> effect) then we'll suffer.
How, exactly, are we suffering from using our deprecated functions?
That's just work we've accidentally deferred until 2.0 when we get rid
of them.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Philip Martin <ph...@codematters.co.uk>.
Julian Foad <ju...@btopenworld.com> writes:
> 1. SVN_NO_IGNORE svn_error_t *
> function_name(param one,
> param two);
> 1 and 2 are (with possibly a different macro name) very clear in
> meaning but a bit long-winded.
I like 1.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Julian Foad <ju...@btopenworld.com>.
> On 1/26/06, Branko Čibej <br...@xbc.nu> wrote:
>
>>Before anyone starts doing this, please consider:
>>
>> * What happens to the generated API documentation
Doxygen seems to do what you want when SVN_ERR_FUNC is defined as
__attribute__((warn_unused_result)) svn_error_t *
That is, for a function declared as "SVN_ERR_FUNC function_name" it just shows
"svn_error_t *function_name" in the docs and "SVN_ERR_FUNC" doesn't appear
anywhere except as a defined symbol and in the source-code views.
>> * Can we conceivably do this with an attribute on svn_error_t
>> instead of every function that returns it
I don't think so, with the current compilers.
David James wrote:
> Here's an alternative proposal: Instead of adding attributes to each
> function manually, we could write a script which adds and removes the
> attributes on demand. If folks want to test for deprecated functions,
> unchecked errors, or other problems, they can use this script.
That's a good idea. I found it quite easy to mostly automate adding
"SVN_ERR_FUNC", and I think it should be possible to fully automate it.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Ivan Zhakov <ch...@gmail.com>.
On 1/27/06, David James <dj...@collab.net> wrote:
> On 1/26/06, Branko Čibej <br...@xbc.nu> wrote:
> > >>> 2. SVN_NO_IGNORE(svn_error_t *)
> > >>> function_name(param one,
> > >>> param two);
> [...]
> > Before anyone starts doing this, please consider:
> >
> > * What happens to the generated API documentation
> > * Can we conceivably do this with an attribute on svn_error_t
> > instead of every function that returns it
> >
> > Whatever you decide to do, the generated docs have to remain clean (that
> > is, there should be no trace of such obfuscation^Wdecoration in the docs).
>
> Here's an alternative proposal: Instead of adding attributes to each
> function manually, we could write a script which adds and removes the
> attributes on demand. If folks want to test for deprecated functions,
> unchecked errors, or other problems, they can use this script. The
> script would work like lint. We could even integrate this script into
> our test suite, if we want.
+1, I dislike decoration macros. They adds ambiguous.
--
Ivan Zhakov
Re: More error leaks
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Thu, 26 Jan 2006, David James wrote:
> On 1/26/06, Branko ?ibej <br...@xbc.nu> wrote:
> >>> 2. SVN_NO_IGNORE(svn_error_t *)
> >>> function_name(param one,
> >>> param two);
[...]
> Before anyone starts doing this, please consider:
>
> * What happens to the generated API documentation
> * Can we conceivably do this with an attribute on svn_error_t
> instead of every function that returns it
>
> Whatever you decide to do, the generated docs have to remain clean (that
> is, there should be no trace of such obfuscation^Wdecoration in the docs).
>Here's an alternative proposal: Instead of adding attributes to each
>function manually, we could write a script which adds and removes the
>attributes on demand. If folks want to test for deprecated functions,
>unchecked errors, or other problems, they can use this script. The
>script would work like lint. We could even integrate this script into
>our test suite, if we want.
We can't have it in our test suite, since it has to be done before
building, but I'm +1 on having a script that does this for casual
sanity checking.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by David James <dj...@collab.net>.
On 1/26/06, Branko Čibej <br...@xbc.nu> wrote:
> >>> 2. SVN_NO_IGNORE(svn_error_t *)
> >>> function_name(param one,
> >>> param two);
[...]
> Before anyone starts doing this, please consider:
>
> * What happens to the generated API documentation
> * Can we conceivably do this with an attribute on svn_error_t
> instead of every function that returns it
>
> Whatever you decide to do, the generated docs have to remain clean (that
> is, there should be no trace of such obfuscation^Wdecoration in the docs).
Here's an alternative proposal: Instead of adding attributes to each
function manually, we could write a script which adds and removes the
attributes on demand. If folks want to test for deprecated functions,
unchecked errors, or other problems, they can use this script. The
script would work like lint. We could even integrate this script into
our test suite, if we want.
Cheers,
David
--
David James -- http://www.cs.toronto.edu/~james
Re: More error leaks
Posted by Branko Čibej <br...@xbc.nu>.
Daniel Rall wrote:
> On Thu, 26 Jan 2006, Garrett Rooney wrote:
>
>
>> On 1/25/06, Julian Foad <ju...@btopenworld.com> wrote:
>>
>>
>>> 2. SVN_NO_IGNORE(svn_error_t *)
>>> function_name(param one,
>>> param two);
>>>
>> I prefer this method, FWIW, since it implies that what you're not
>> supposed to ignore is the svn_error_t * you get from the function, and
>> more importantly because it avoids obscuring the return value.
>>
>
> I lean towards not marking up the function decls at all, but if
> consensus goes the other way, I also prefere this one. As Garrett
> mentions, it obscures what's going on the least.
>
Before anyone starts doing this, please consider:
* What happens to the generated API documentation
* Can we conceivably do this with an attribute on svn_error_t
instead of every function that returns it
Whatever you decide to do, the generated docs have to remain clean (that
is, there should be no trace of such obfuscation^Wdecoration in the docs).
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Daniel Rall <dl...@collab.net>.
On Thu, 26 Jan 2006, Garrett Rooney wrote:
> On 1/25/06, Julian Foad <ju...@btopenworld.com> wrote:
>
> > 2. SVN_NO_IGNORE(svn_error_t *)
> > function_name(param one,
> > param two);
>
> I prefer this method, FWIW, since it implies that what you're not
> supposed to ignore is the svn_error_t * you get from the function, and
> more importantly because it avoids obscuring the return value.
I lean towards not marking up the function decls at all, but if
consensus goes the other way, I also prefere this one. As Garrett
mentions, it obscures what's going on the least.
--
Daniel Rall
Re: More error leaks
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 1/25/06, Julian Foad <ju...@btopenworld.com> wrote:
> 2. SVN_NO_IGNORE(svn_error_t *)
> function_name(param one,
> param two);
I prefer this method, FWIW, since it implies that what you're not
supposed to ignore is the svn_error_t * you get from the function, and
more importantly because it avoids obscuring the return value.
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Julian Foad <ju...@btopenworld.com>.
I (Julian Foad) wrote:
> That just leaves Greg's point that my macro obscures the return value.
Sorry, I meant Garrett's point.
> Well, yes, it does. It wouldn't matter if we never referred to
> "svn_error_t" elsewhere, but we do, in this context:
>
> svn_error_t *err = fn(...);
>> Peter Samuelson wrote:
>>
>>> If I had a vote, I would vote for:
>>>
>>> #define SVN_ERROR_T __attribute__((warn_unused_result)) svn_error_t
>>>
>>> Then it's just a matter of replacing svn_error_t with SVN_ERROR_T.
>>> That seems pretty readable to me.
That's an interesting idea. It is certainly "pretty" at the point of use, and
gives a clear reminder of the actual return type. It is, however, rather
illogical in grouping the attribute with just part of the return type.
> Greg Hudson wrote:
>> My vote: don't mark up the source code like this at all. It has value
>> as a debugging aid, but it has greater harm in scaring people away from
>> the source code.
That's definitely a serious consideration. For a while you made me think this
is not worth doing if it is going to cause any such harm. Then I went on to
check the usage of our "deprecated" functions and found another whole class of
errors that could have been automatically detected. If we don't take the
opportunity to use automated checks whenever we can (without too much adverse
effect) then we'll suffer. The project is too complex for a human to
reasonably check these sorts of things.
Thus I have swayed back in favour of employing this automated check. The only
question for me now is which particular form we would like best.
1. SVN_NO_IGNORE svn_error_t *
function_name(param one,
param two);
2. SVN_NO_IGNORE(svn_error_t *)
function_name(param one,
param two);
3. SVN_ERR_FUNC function_name(param one,
param two);
4. SVN_ERROR_T *function_name(param one,
param two);
1 and 2 are (with possibly a different macro name) very clear in meaning but a
bit long-winded.
2 and 3 have more potential to support other compilers than 1 and 4.
3 is the form that I like best, though it obscures the return type and I don't
actually think the name "SVN_ERR_FUNC" is a good description. "SVN_ERR_RETURN"
is probably better. Choosing a good name is more important in the long term
than keeping the length so that the indentation doesn't change.
4 is neatest at the point of use but rather illogical.
Branko Čibej wrote:
> +1. We have valgrind, after all.
I think that's a red herring. Neither valgrind nor any run-time analysis can
find leaks that don't occur in situations created by the tester, as shown by
the fact that nobody had found the couple of dozen leaks that this method found.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Branko Čibej <br...@xbc.nu>.
Greg Hudson wrote:
> On Wed, 2006-01-25 at 03:35 -0600, Peter Samuelson wrote:
>
>> If I had a vote, I would vote for:
>>
>> #define SVN_ERROR_T __attribute__((warn_unused_result)) svn_error_t
>>
>> Then it's just a matter of replacing svn_error_t with SVN_ERROR_T.
>> That seems pretty readable to me.
>>
>
> My vote: don't mark up the source code like this at all. It has value
> as a debugging aid, but it has greater harm in scaring people away from
> the source code.
>
+1. We have valgrind, after all.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Greg Hudson <gh...@MIT.EDU>.
On Wed, 2006-01-25 at 03:35 -0600, Peter Samuelson wrote:
> If I had a vote, I would vote for:
>
> #define SVN_ERROR_T __attribute__((warn_unused_result)) svn_error_t
>
> Then it's just a matter of replacing svn_error_t with SVN_ERROR_T.
> That seems pretty readable to me.
My vote: don't mark up the source code like this at all. It has value
as a debugging aid, but it has greater harm in scaring people away from
the source code.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Peter Samuelson <pe...@p12n.org>.
[Julian Foad]
> +/** A declaration of the return type of a function that returns a
> Subversion
> + * error object.
> + *
> + * @since New in 1.4.
> + */
> +#define SVN_ERR_FUNC __attribute__((warn_unused_result)) svn_error_t *
If I had a vote, I would vote for:
#define SVN_ERROR_T __attribute__((warn_unused_result)) svn_error_t
Then it's just a matter of replacing svn_error_t with SVN_ERROR_T.
That seems pretty readable to me.
Re: More error leaks
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 1/24/06, Julian Foad <ju...@btopenworld.com> wrote:
> I decided to include the return type in the macro, mainly for brevity at the
> point of use, but justified by the idea that the return type and the
> instruction not to ignore it are closely associated with each other. For
> example, the equivalent instruction to a different compiler might well require
> being placed before or after or around the return type:
>
> __no_ignore(svn_error_t *) f(...);
> svn_error_t * _Pragma(no_ignore) f(...);
>
> Of course some compiler might require such an instruction to come after the
> function name, which this wouldn't support. I do NOT believe it is very
> important to try to make other compilers do this same checking. The more
> checks the better, but some compilers will always check things that others
> don't, and that's fine.
>
> How do people feel about me committing this? The patch I have ready covers all
> (1200 or so) declarations in header files. I suppose I should do the same for
> all the (1700 or so) "static" functions too - leaks from them are no less
> important.
Honestly, I'm not a huge fan of the macro including the svn_error_t *,
it makes the headers harder to read. If we must have such a thing,
I'd prefer something like SVN_DECLARE(svn_error_t *) or something,
similar to the way that the APR_DECLARE stuff is done,s ince at least
you can still see the return value. On the other hand, I belive
Branko was against the presence of such a macro when one was suggested
to deal with the windows DLL build issues, so that might have its own
issues...
-garrett
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Wed, Jan 25, 2006 at 12:14:57AM +0000, Julian Foad wrote:
> There are a few other functions where ignoring the return value guarantees
> a resource leak, but not many because of our wide use of pools.
That's a very good point. Okay, I've no objection to tying the two things
(type, attribute) together. In fact, I think it's probably better, since
it makes it harder to forget the attribute in the function definition.
> That just leaves Greg's point that my macro obscures the return value.
> Well, yes, it does. It wouldn't matter if we never referred to
> "svn_error_t" elsewhere, but we do, in this context:
>
> svn_error_t *err = fn(...);
>
> I was about to suggest another macro, something like SVN_ERR_TYPE to pair
> with SVN_ERR_FUNC, to be used here, but I don't think that would be an
> improvement.
>
Agreed. I don't think that would be an improvement either.
> >[That brings up and intreresting point actually: what does gcc < 3.4 do
> >with the attribute? Does it ignore it okay?]
>
> I imagine so. I had no idea this feature was so new. Could someone check
> that?
>
Just did. On gcc 3.3.6, you get:
foo.c:2: warning: `warn_unused_result' attribute directive ignored
So I'd think we'd to conditionalise on the gcc version.
> I think we should annotate everything - declarations and definitions -
> because that would be easier to understand and get right. GCC doesn't seem
> to care whether the definitions are annotated if the declarations are.
>
Good; simpler is better.
> Personally I find this style tolerable if necessary, but much worse than
> the style that used a single macro for "this returns an error object". I
> like the way it emphasised how this is our implementation of "throwing" an
> exception. Maybe we should indicate that with the macro name:
>
> SVN_THROW_FN svn_mime_type_validate (const char *mime_type,
> apr_pool_t *pool);
>
Please don't include 'throw' in the name, it's not C++. I think
SVN_ERR_FUNC was better at at least attempting to describe the return
value (plus, we don't really use 'throw' elsewhere).
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Julian Foad <ju...@btopenworld.com>.
Malcolm Rowe wrote:
> On Tue, Jan 24, 2006 at 12:38:58PM +0000, Julian Foad wrote:
>
>>Most of our functions (over 1200 of them) return "svn_error_t *", and I
>>think writing "__attribute__((warn_unused_result))" literally on every such
>>function would be far too ugly.
>
> Agreed.
>
>
>>+#define SVN_ERR_FUNC __attribute__((warn_unused_result)) svn_error_t *
>
> Nice, and I like the attention to detail wrt the indentation. However,
> as Greg points out, this does obscure the return value (and who's to
> say we wouldn't want to use this with other return types?
To address your parenthesis: first, why wouldn't we want to force some other
return types to be used?
Ignoring an "svn_error_t" always introduces a bug, and that bug is not detected
by normal testing as it is not a "correctness" bug. There are many functions
where ignoring the return value means the program surely can't be right, but
the error would show up as a correctness bug for that particular part of the
program, and I don't think it is useful to apply the "must be used" warning in
those cases. It might be of some use, but I suspect it would be difficult to
decide in several cases whether to require the return value to be used, and in
many cases we would wish one or more return-through-pointer parameters to be
treated as "must be used" and we can't do that, so we'd have a very incomplete
system of checks.
Secondly, what are the consequences if we do want to apply the warning to other
types?
There are a few other functions where ignoring the return value guarantees a
resource leak, but not many because of our wide use of pools. Let's say we
wanted to force the use of the return value of a function that creates a file
(not connected to a pool) and returns the only reference to it. I would
advocate either using a macro such as "SVN_FILE_RETURN" along the lines of this
proposal (if there are several such functions), or else using some macro such
as "SVN_NO_IGNORE" in addition to the return type. Neither of those solutions
conflicts with the present proposal so I think your parenthesis is moot.
That just leaves Greg's point that my macro obscures the return value. Well,
yes, it does. It wouldn't matter if we never referred to "svn_error_t"
elsewhere, but we do, in this context:
svn_error_t *err = fn(...);
I was about to suggest another macro, something like SVN_ERR_TYPE to pair with
SVN_ERR_FUNC, to be used here, but I don't think that would be an improvement.
>
> We could do something similar to the Linux kernel, and define a keyword
> that expands to the syntax we need.
>
> LXR has a copy of the 2.6.11 source, which declares (in a GCC3-specific file):
>
> #if __GNUC_MINOR__ >= 4
> #define __must_check __attribute__((warn_unused_result))
Sure - there's absolutely no problem with that approach if we accept the
addition of "__must_check" or "SVN_NO_IGNORE" or whatever at every point of
use. I'm not worried about the mechanics of getting a macro defined
appropriately; however, I don't like the visual clutter at the point of use.
> [That brings up and intreresting point actually: what does gcc < 3.4 do
> with the attribute? Does it ignore it okay?]
I imagine so. I had no idea this feature was so new. Could someone check that?
> Then again, I see the value of making it hard to declare a function
> returning an svn_error_t* without also declaring it __must_check or
> equivalent.
Hmm, yes. I hadn't really thought of that but it is a significant point.
[...]
>>How do people feel about me committing this? The patch I have ready covers
>>all (1200 or so) declarations in header files. I suppose I should do the
>>same for all the (1700 or so) "static" functions too - leaks from them are
>>no less important.
>
> Do we annotate just the declarations (in public header files and statics),
> and also all the static definitions that lack declarations? Or do we just
> annotate everything in sight, it being easier? Does it matter to gcc?
I think we should annotate everything - declarations and definitions - because
that would be easier to understand and get right. GCC doesn't seem to care
whether the definitions are annotated if the declarations are.
> Do we bump the copyright dates on all the files too? (With my IANAL
> hat firmly on, I'd guess not: this is a mechanical change, not one that
> introduces anything new).
That sounds reasonable.
I've just converted my source tree to use "SVN_NO_IGNORE svn_error_t *"
throughout, to see how it looks. It involved splitting each function name onto
a separate line from its return type, in the many cases where that wasn't
already done, and re-indenting the parameter list. (Didn't take long in Vim.)
This is the equivalent of the snippet I showed last time:
Index: subversion/include/svn_types.h
===================================================================
...
+/** ...
+ *
+ * @since New in 1.4.
+ */
+#define SVN_NO_IGNORE __attribute__((warn_unused_result))
+
...
-svn_error_t *svn_mime_type_validate (const char *mime_type,
- apr_pool_t *pool);
+SVN_NO_IGNORE svn_error_t *
+svn_mime_type_validate (const char *mime_type,
+ apr_pool_t *pool);
...
-typedef svn_error_t *(*svn_cancel_func_t) (void *cancel_baton);
+typedef SVN_NO_IGNORE svn_error_t *
+(*svn_cancel_func_t) (void *cancel_baton);
Index: subversion/libsvn_fs_base/trail.h
===================================================================
...
-svn_error_t *
+SVN_NO_IGNORE svn_error_t *
svn_fs_base__retry_debug (svn_fs_t *fs,
- svn_error_t *(*txn_body) (void *baton,
- trail_t *trail),
+ SVN_NO_IGNORE svn_error_t *
+ (*txn_body) (void *baton,
+ trail_t *trail),
void *baton,
...
-svn_error_t *svn_fs_base__retry (svn_fs_t *fs,
- svn_error_t *(*txn_body) (void *baton,
- trail_t *trail),
- void *baton,
- apr_pool_t *pool);
+SVN_NO_IGNORE svn_error_t *
+svn_fs_base__retry (svn_fs_t *fs,
+ SVN_NO_IGNORE svn_error_t *
+ (*txn_body) (void *baton,
+ trail_t *trail),
+ void *baton,
+ apr_pool_t *pool);
...
(Note: these last two declarations, each containing an in-line declaration of a
"txn_body" function type, would be better if that function type were defined
out of line. That's a separate issue.)
Personally I find this style tolerable if necessary, but much worse than the
style that used a single macro for "this returns an error object". I like the
way it emphasised how this is our implementation of "throwing" an exception.
Maybe we should indicate that with the macro name:
SVN_THROW_FN svn_mime_type_validate (const char *mime_type,
apr_pool_t *pool);
So I'm still awaiting others' thoughts.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Tue, Jan 24, 2006 at 12:38:58PM +0000, Julian Foad wrote:
> Most of our functions (over 1200 of them) return "svn_error_t *", and I
> think writing "__attribute__((warn_unused_result))" literally on every such
> function would be far too ugly.
Agreed.
> Yes, I know (now that Philip reminded me)
> that we have "__attribute__((format(printf,...))" on some functions, but
> only a dozen or so. Certainly, if someone suggested adding such a long,
> compiler-specific annotation for, say, a Microsoft compiler, to a large
> number of functions, I would be one of the first to object. Therefore I
> suggest this instead:
>
> +#define SVN_ERR_FUNC __attribute__((warn_unused_result)) svn_error_t *
Nice, and I like the attention to detail wrt the indentation. However,
as Greg points out, this does obscure the return value (and who's to
say we wouldn't want to use this with other return types?
We could do something similar to the Linux kernel, and define a keyword
that expands to the syntax we need.
LXR has a copy of the 2.6.11 source, which declares (in a GCC3-specific file):
#if __GNUC_MINOR__ >= 4
#define __must_check __attribute__((warn_unused_result))
#endif
[http://lxr.linux.no/source/include/linux/compiler-gcc3.h]
__must_check is later conditionally defined nothing if it's not yet
been defined.
[That brings up and intreresting point actually: what does gcc < 3.4 do
with the attribute? Does it ignore it okay?]
Then again, I see the value of making it hard to declare a function
returning an svn_error_t* without also declaring it __must_check or
equivalent.
> I do NOT believe it is very
> important to try to make other compilers do this same checking. The more
> checks the better, but some compilers will always check things that others
> don't, and that's fine.
Agreed, and that's why I'm not too bothered about hard-coding the position
of the token.
> How do people feel about me committing this? The patch I have ready covers
> all (1200 or so) declarations in header files. I suppose I should do the
> same for all the (1700 or so) "static" functions too - leaks from them are
> no less important.
>
Do we annotate just the declarations (in public header files and statics),
and also all the static definitions that lack declarations? Or do we just
annotate everything in sight, it being easier? Does it matter to gcc?
Do we bump the copyright dates on all the files too? (With my IANAL
hat firmly on, I'd guess not: this is a mechanical change, not one that
introduces anything new).
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Julian Foad <ju...@btopenworld.com>.
Malcolm Rowe wrote:
> On Mon, Jan 23, 2006 at 06:21:49PM +0000, Philip Martin wrote:
>
>>>>Of course the other question raised is: do we want to consider using such
>>>>attributes as a matter of course? Obviously only if it is done neatly and
>>>>in a way that doesn't interfere with other compilers; macros can help to
>>>>achieve that. I'll have a bit more of a think about that.
>>>
>>>Yes, we should do this by default, if we can. Any idea how?
>>
>>No problem, we already use __attribute__ in the public API.
>
> So do we add a macro that expands to '__attribute__(...) svn_error_t *'
> and adjust all the existing function declarations/definitions to use that?
> That's the obvious solution, but I was hoping that Julian might be able
> to come up with something more cunning ;-)
Sorry, I have nothing more cunning. In fact, I thought of exactly what you did.
Most of our functions (over 1200 of them) return "svn_error_t *", and I think
writing "__attribute__((warn_unused_result))" literally on every such function
would be far too ugly. Yes, I know (now that Philip reminded me) that we have
"__attribute__((format(printf,...))" on some functions, but only a dozen or so.
Certainly, if someone suggested adding such a long, compiler-specific
annotation for, say, a Microsoft compiler, to a large number of functions, I
would be one of the first to object. Therefore I suggest this instead:
Index: subversion/include/svn_types.h
===================================================================
...
+/** A declaration of the return type of a function that returns a Subversion
+ * error object.
+ *
+ * @since New in 1.4.
+ */
+#define SVN_ERR_FUNC __attribute__((warn_unused_result)) svn_error_t *
+
...
-svn_error_t *svn_mime_type_validate (const char *mime_type,
+SVN_ERR_FUNC svn_mime_type_validate (const char *mime_type,
apr_pool_t *pool);
...
-typedef svn_error_t *(*svn_cancel_func_t) (void *cancel_baton);
+typedef SVN_ERR_FUNC (*svn_cancel_func_t) (void *cancel_baton);
Index: subversion/libsvn_fs_base/trail.h
===================================================================
...
-svn_error_t *
+SVN_ERR_FUNC
svn_fs_base__retry_debug (svn_fs_t *fs,
- svn_error_t *(*txn_body) (void *baton,
+ SVN_ERR_FUNC (*txn_body) (void *baton,
trail_t *trail),
void *baton,
...
-svn_error_t *svn_fs_base__retry (svn_fs_t *fs,
- svn_error_t *(*txn_body) (void *baton,
+SVN_ERR_FUNC svn_fs_base__retry (svn_fs_t *fs,
+ SVN_ERR_FUNC (*txn_body) (void *baton,
trail_t *trail),
void *baton,
...
Those are just a few snippets from a huge diff to illustrate how this applies
to various layouts of prototypes and typedefs. I chose the name "SVN_ERR_FUNC"
to have a length that matches the existing indentation in the vast majority of
cases.
I decided to include the return type in the macro, mainly for brevity at the
point of use, but justified by the idea that the return type and the
instruction not to ignore it are closely associated with each other. For
example, the equivalent instruction to a different compiler might well require
being placed before or after or around the return type:
__no_ignore(svn_error_t *) f(...);
svn_error_t * _Pragma(no_ignore) f(...);
Of course some compiler might require such an instruction to come after the
function name, which this wouldn't support. I do NOT believe it is very
important to try to make other compilers do this same checking. The more
checks the better, but some compilers will always check things that others
don't, and that's fine.
How do people feel about me committing this? The patch I have ready covers all
(1200 or so) declarations in header files. I suppose I should do the same for
all the (1700 or so) "static" functions too - leaks from them are no less
important.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Jan 23, 2006 at 07:53:20PM +0000, Philip Martin wrote:
> > So do we add a macro that expands to '__attribute__(...) svn_error_t *'
> > and adjust all the existing function declarations/definitions to use that?
>
> No need for a macro, simply add __attribute__(...) to the function
> prototype. It should work since we already do that for other
> attributes.
>
I was thinking of a macro in case we later needed to change the expansion
(for example, if gcc changed the attribute we needed to use).
Do we need to adjust both the declaration and definition, or is one
enough?
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 23 Jan 2006, Philip Martin wrote:
> "Peter N. Lundblad" <pe...@famlundblad.se> writes:
>
> > On Mon, 23 Jan 2006, Philip Martin wrote:
> >> No need for a macro, simply add __attribute__(...) to the function
> >> prototype. It should work since we already do that for other
> >> attributes.
> >>
> > Yes, it should, but to me it looks a little clumsy. I'd prefer a macro
> > instead.
>
> I'm surprised, I can't see how a macro would be an improvement.
>
Somewhat shorter, no __attribute__ in lots of places. We will have these
on nearly every function. I guess this is mostly a matter of taste, but
like to hide compiler-specific stuff using macros.
There's also the argument that some other compiler might have this feature
with a different syntax, but I don't know of any, so that's nearly moot.
Thanks,
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Philip Martin <ph...@codematters.co.uk>.
"Peter N. Lundblad" <pe...@famlundblad.se> writes:
> On Mon, 23 Jan 2006, Philip Martin wrote:
>> No need for a macro, simply add __attribute__(...) to the function
>> prototype. It should work since we already do that for other
>> attributes.
>>
> Yes, it should, but to me it looks a little clumsy. I'd prefer a macro
> instead.
I'm surprised, I can't see how a macro would be an improvement.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by "Peter N. Lundblad" <pe...@famlundblad.se>.
On Mon, 23 Jan 2006, Philip Martin wrote:
> Malcolm Rowe <ma...@farside.org.uk> writes:
>
> >> No problem, we already use __attribute__ in the public API.
> >
> > So do we add a macro that expands to '__attribute__(...) svn_error_t *'
> > and adjust all the existing function declarations/definitions to use that?
>
> No need for a macro, simply add __attribute__(...) to the function
> prototype. It should work since we already do that for other
> attributes.
>
Yes, it should, but to me it looks a little clumsy. I'd prefer a macro
instead.
//Peter
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Philip Martin <ph...@codematters.co.uk>.
Malcolm Rowe <ma...@farside.org.uk> writes:
>> No problem, we already use __attribute__ in the public API.
>
> So do we add a macro that expands to '__attribute__(...) svn_error_t *'
> and adjust all the existing function declarations/definitions to use that?
No need for a macro, simply add __attribute__(...) to the function
prototype. It should work since we already do that for other
attributes.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Mon, Jan 23, 2006 at 06:21:49PM +0000, Philip Martin wrote:
> > The ones I didn't fix were:
> >
> >> subversion/libsvn_diff/diff_file.c: In function
> >> 'svn_diff__file_output_unified_default_hdr':
> >> subversion/libsvn_diff/diff_file.c:894: return value of 'svn_io_stat'
> >
> > .. because the function 'can't fail', and:
>
> By "can't fail" do you mean "the calling function doesn't return an
> error"? The svn_io_stat call can certainly fail so at the very least
> do:
>
> svn_error_clear (svn_io_stat (...));
>
> or
>
> err = svn_io_stat (...);
> if (err)
> {
> file_info.mtime = 0;
> svn_error_clear (err);
> }
>
By 'can't fail', I meant that svn_diff__file_output_unified_default_hdr()
doesn't provide any way to communicate an error to its caller, and so I
wasn't sure how best to fix the leak offhand.
Since it's static, it should be easy enough to adjust the function to
return an svn_error_t*, then we can add in an SVN_ERR() macro. I think
that's preferable to ignoring the error, no?
> >> Of course the other question raised is: do we want to consider using such
> >> attributes as a matter of course? Obviously only if it is done neatly and
> >> in a way that doesn't interfere with other compilers; macros can help to
> >> achieve that. I'll have a bit more of a think about that.
> >>
> >
> > Yes, we should do this by default, if we can. Any idea how?
>
> No problem, we already use __attribute__ in the public API.
>
So do we add a macro that expands to '__attribute__(...) svn_error_t *'
and adjust all the existing function declarations/definitions to use that?
That's the obvious solution, but I was hoping that Julian might be able
to come up with something more cunning ;-)
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Philip Martin <ph...@codematters.co.uk>.
Malcolm Rowe <ma...@farside.org.uk> writes:
> On Thu, Jan 19, 2006 at 04:10:21PM +0000, Julian Foad wrote:
>> By putting "__attribute__((warn_unused_result))" on each declaration of a
>> function that returns "svn_error_t *", I found the following errors being
>> leaked:
>>
>
> Fantastic job, Julian! I fixed all the trivial instances in r18196.
>
> The ones I didn't fix were:
>
>> subversion/libsvn_diff/diff_file.c: In function
>> 'svn_diff__file_output_unified_default_hdr':
>> subversion/libsvn_diff/diff_file.c:894: return value of 'svn_io_stat'
>
> .. because the function 'can't fail', and:
By "can't fail" do you mean "the calling function doesn't return an
error"? The svn_io_stat call can certainly fail so at the very least
do:
svn_error_clear (svn_io_stat (...));
or
err = svn_io_stat (...);
if (err)
{
file_info.mtime = 0;
svn_error_clear (err);
}
>> subversion/svn/main.c: In function 'main':
>> subversion/svnadmin/main.c: In function 'main':
>> subversion/svndumpfilter/main.c: In function 'main':
>> subversion/svnlook/main.c: In function 'main':
>> subversion/svnsync/main.c: In function 'main':
>
> .. because the main functions need something slightly more complex
> (a call to svn_cmdline_handle_exit_error(), perhaps).
>
> So those still need some attention.
>
>> Of course the other question raised is: do we want to consider using such
>> attributes as a matter of course? Obviously only if it is done neatly and
>> in a way that doesn't interfere with other compilers; macros can help to
>> achieve that. I'll have a bit more of a think about that.
>>
>
> Yes, we should do this by default, if we can. Any idea how?
No problem, we already use __attribute__ in the public API.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Julian Foad <ju...@btopenworld.com>.
Malcolm Rowe wrote:
> On Thu, Jan 19, 2006 at 04:10:21PM +0000, Julian Foad wrote:
>
>>By putting "__attribute__((warn_unused_result))" on each declaration of a
>>function that returns "svn_error_t *", I found the following errors being
>>leaked:
>
> Fantastic job, Julian! I fixed all the trivial instances in r18196.
>
> The ones I didn't fix were:
>
>>subversion/libsvn_diff/diff_file.c: In function
>>'svn_diff__file_output_unified_default_hdr':
>>subversion/libsvn_diff/diff_file.c:894: return value of 'svn_io_stat'
>
> .. because the function 'can't fail', and:
>
>>subversion/svn/main.c: In function 'main':
>>subversion/svnadmin/main.c: In function 'main':
>>subversion/svndumpfilter/main.c: In function 'main':
>>subversion/svnlook/main.c: In function 'main':
>>subversion/svnsync/main.c: In function 'main':
>
> .. because the main functions need something slightly more complex
> (a call to svn_cmdline_handle_exit_error(), perhaps).
>
> So those still need some attention.
I fixed the one in diff_file.c in r18446. The ones in the various 'main'
functions aren't so important, though I might do them later. (There's no
end-user benefit to cleaning up just before exiting the program, only developer
benefits like learning by example, clean use of 'valgrind', etc.)
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Wed, Jan 25, 2006 at 12:29:30AM +0000, Julian Foad wrote:
> Malcolm Rowe wrote:
> >Fantastic job, Julian! I fixed all the trivial instances in r18196.
>
> Thanks, Malcolm. Now I've done the same on static functions and found four
> more (with my comments):
>
>
> If you'd care to fix those too that would be nice.
>
r18249. Note that someone still needs to fix the non-trivial cases from
your first message.
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Julian Foad <ju...@btopenworld.com>.
Malcolm Rowe wrote:
> On Thu, Jan 19, 2006 at 04:10:21PM +0000, Julian Foad wrote:
>
>>By putting "__attribute__((warn_unused_result))" on each declaration of a
>>function that returns "svn_error_t *", I found the following errors being
>>leaked:
>
> Fantastic job, Julian! I fixed all the trivial instances in r18196.
Thanks, Malcolm. Now I've done the same on static functions and found four
more (with my comments):
subversion/libsvn_wc/adm_files.c: In function 'init_adm':
subversion/libsvn_wc/adm_files.c:1037: return value of 'make_empty_adm'
Should use SVN_ERR.
--
subversion/libsvn_wc/entries.c: In function 'svn_wc__entries_write':
subversion/libsvn_wc/entries.c:1287: return value of 'write_entry'
Should return "void".
--
subversion/svndumpfilter/main.c: In function ‘new_node_record’:
subversion/svndumpfilter/main.c:529: return value of 'output_revision'
Should use SVN_ERR.
--
subversion/mod_dav_svn/update.c: In function 'upd_open_directory':
subversion/mod_dav_svn/update.c:739: return value of 'open_helper'
Caller should just return the error, like the nearby functions do.
If you'd care to fix those too that would be nice.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: More error leaks
Posted by Malcolm Rowe <ma...@farside.org.uk>.
On Thu, Jan 19, 2006 at 04:10:21PM +0000, Julian Foad wrote:
> By putting "__attribute__((warn_unused_result))" on each declaration of a
> function that returns "svn_error_t *", I found the following errors being
> leaked:
>
Fantastic job, Julian! I fixed all the trivial instances in r18196.
The ones I didn't fix were:
> subversion/libsvn_diff/diff_file.c: In function
> 'svn_diff__file_output_unified_default_hdr':
> subversion/libsvn_diff/diff_file.c:894: return value of 'svn_io_stat'
.. because the function 'can't fail', and:
> subversion/svn/main.c: In function 'main':
> subversion/svnadmin/main.c: In function 'main':
> subversion/svndumpfilter/main.c: In function 'main':
> subversion/svnlook/main.c: In function 'main':
> subversion/svnsync/main.c: In function 'main':
.. because the main functions need something slightly more complex
(a call to svn_cmdline_handle_exit_error(), perhaps).
So those still need some attention.
> Of course the other question raised is: do we want to consider using such
> attributes as a matter of course? Obviously only if it is done neatly and
> in a way that doesn't interfere with other compilers; macros can help to
> achieve that. I'll have a bit more of a think about that.
>
Yes, we should do this by default, if we can. Any idea how?
Regards,
Malcolm
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org