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).
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.
Received on 2013-04-04 23:18:35 CEST