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
Received on Thu Jan 26 03:26:13 2006