Hi Branko,
On Thu, Apr 4, 2013 at 8:59 PM, Branko Čibej <brane_at_wandisco.com> wrote:
> On 04.04.2013 20:46, Lieven Govaerts wrote:
>> On Thu, Apr 4, 2013 at 1:28 PM, Philip Martin
>> <philip.martin_at_wandisco.com> wrote:
>>> Lieven Govaerts <lgo_at_apache.org> writes:
>>>
>>>> On Thu, Apr 4, 2013 at 12:55 AM, philip_at_apache.org <philip_at_apache.org> wrote:
>>>>> Author: philip
>>>>> Date: Wed Apr 3 22:55:37 2013
>>>>> New Revision: 1464228
>>>>>
>>>>> URL: http://svn.apache.org/r1464228
>>>>> Log:
>>>>> Remove (void) casts of ignored return values from ra_serf.
>>>>>
>>>>> * subversion/libsvn_ra_serf/util.c
>>>>> (svn_ra_serf__conn_closed, svn_ra_serf__process_pending,
>>>>> svn_ra_serf__handle_xml_parser, svn_ra_serf__credentials_callback,
>>>>> svn_ra_serf__request_create, expat_start, expat_end,
>>>>> expat_cdata, expat_response_handler): Remove cast.
>>>> What's the benefit of removing these casts? Is the purpose to consider
>>>> these as warnings in order to solve them later?
>>> We were not consistently casting to (void) when ignoring return values.
>>> Branko objected when I added such a cast recently so I decided to remove
>>> them all.
>>>
>>>>> Modified: subversion/trunk/subversion/libsvn_ra_serf/util.c
>>>>> URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_ra_serf/util.c?rev=1464228&r1=1464227&r2=1464228&view=diff
>>>>> ==============================================================================
>>>>> --- subversion/trunk/subversion/libsvn_ra_serf/util.c (original)
>>>>> +++ subversion/trunk/subversion/libsvn_ra_serf/util.c Wed Apr 3 22:55:37 2013
>>>>> @@ -497,7 +497,7 @@ svn_ra_serf__conn_closed(serf_connection
>>>>>
>>>>> err = svn_error_trace(connection_closed(ra_conn, why, pool));
>>>>>
>>>>> - (void) save_error(ra_conn->session, err);
>>>>> + save_error(ra_conn->session, err);
>>>>> }
>>>>>
>>>>>
>>>>> @@ -1490,7 +1490,7 @@ svn_ra_serf__process_pending(svn_ra_serf
>>>>>
>>>>> /* Tell the parser that no more content will be parsed. Ignore the
>>>>> return status. We just don't care. */
>>>>> - (void) XML_Parse(parser->xmlp, NULL, 0, 1);
>>>>> + XML_Parse(parser->xmlp, NULL, 0, 1);
>>>> This confuses me a bit. You leave the comment so I guess you agree
>>>> with the decision to ignore the return value here, yet you remove the
>>>> (void) cast which makes the same thing clear from the code.
>>> If the (void) cast helps then we should be using it everywhere we ignore
>>> a return value.
>> Using (void) says the same thing as /* Ignore the return status. We
>> just don't care. */ but in a more compact and precise manner.
>> I see no reason to not use it, in those cases where we're absolutely
>> sure the return value is not needed of course.
>
> Using (void) was invented to shut up Lint and its derivatives, back in
> the day. It's ugly and unnecessary and doesn't tell the compiler
> anything it doesn't already know. I see every reason not to use it -- it
> clutters up the code in a way that distracts from the overall meaning.
>
This is not about communication to the compiler, but to the person
reading the code. It says that the original developer of that code
knows that the function returns something, but that (s)he knowingly
decide to ignore it.
I don't see why that's not valuable information. Do you really think
that comments are a better way to express this decision? I don't.
Lieven
Received on 2013-04-04 22:04:27 CEST