On 04.04.2013 23:17, Daniel Shahaf wrote:
> Lieven Govaerts wrote on Thu, Apr 04, 2013 at 22:03:35 +0200:
>> 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.
> For example, this implies we won't use (void) in front of printf()
> calls (even if that would leave lint complaining).
Well of course you won't, who on earth ever checks the return value of
printf()? In some rare cases you do, but not usually. Unlike, e.g.,
svn_cmdline_printf, which we /do/ check return values from.
> FWIW, lgo's argument makes sense to me. I see no reason to remove
> (void) casts that make our code more readable because some other
> codebases use them to silence lint.
My point is that they do not make the code more readable. Moreover: when
someone does turn on the extra compiler options to find places where
return values are ignored, the (void)-cast calls will not be flagged.
Who's to guarantee that the original author who decided to ignore the
return value and dropped the (void) tu^Wbreadcrumb was right? What if
the semantics of the called function, or the calling code, changes in
such a way that ignoring the return value is no longer valid?
And to respond to Lieven: yes, I (unprintable) well do expect an
explanation for dropping the return value to the floor in a comment, if
it's not for a trivially obvious reason. The (void) cast only says "I'm
ignoring the return value," which is hardly useful information since
anyone can see that even without the cast. Which does not imply that
each and every such statement in the code needs its own comment.
-- Brane
--
Branko Čibej
Director of Subversion | WANdisco | www.wandisco.com
Received on 2013-04-05 08:15:14 CEST