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
Received on Wed Jan 25 01:15:24 2006