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
Received on Tue Jan 24 13:40:09 2006