[PATCH] Warning for main returning a bool.

Mon Nov 28 17:45:22 PST 2016

Committed as r288097.

On 28 November 2016 at 05:27, Joshua Hurwitz via cfe-commits <
cfe-commits at lists.llvm.org> wrote:

> Thanks Richard for looking at the revised patch.
>
> On Mon, Nov 21, 2016 at 1:50 PM Richard Smith <richard at metafoo.co.uk>
> wrote:
>
>> This looks good to me. (While we could generalize this further, this
>> patch is a strict improvement, and we'll probably want to treat the 'main'
>> case specially regardless of whether we add a more general conversion
>> warning.)
>>
>> On 21 November 2016 at 07:18, Joshua Hurwitz <hurwitzj at google.com> wrote:
>>
>> I modified the patch to warn only when a bool literal is returned from
>> main. See attached. -Josh
>>
>>
>> On Tue, Nov 15, 2016 at 7:50 PM Richard Smith <richard at metafoo.co.uk>
>> wrote:
>>
>> On Tue, Nov 15, 2016 at 2:55 PM, Aaron Ballman via cfe-commits <
>> cfe-commits at lists.llvm.org> wrote:
>>
>> On Tue, Nov 15, 2016 at 5:44 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>> > ----- Original Message -----
>> >> From: "Aaron Ballman" <aaron at aaronballman.com>
>> >> To: "Hal Finkel" <hfinkel at anl.gov>
>> >> Cc: "cfe-commits" <cfe-commits at lists.llvm.org>, "Joshua Hurwitz" <
>> hurwitzj at google.com>
>> >> Sent: Tuesday, November 15, 2016 4:42:05 PM
>> >> Subject: Re: [PATCH] Warning for main returning a bool.
>> >>
>> >> On Tue, Nov 15, 2016 at 5:22 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>> >> > ----- Original Message -----
>> >> >> From: "Aaron Ballman via cfe-commits" <cfe-commits at lists.llvm.org>
>> >> >> To: "Joshua Hurwitz" <hurwitzj at google.com>
>> >> >> Cc: "cfe-commits" <cfe-commits at lists.llvm.org>
>> >> >> Sent: Tuesday, November 15, 2016 12:17:28 PM
>> >> >> Subject: Re: [PATCH] Warning for main returning a bool.
>> >> >>
>> >> >> On Fri, Oct 14, 2016 at 1:17 PM, Joshua Hurwitz via cfe-commits
>> >> >> <cfe-commits at lists.llvm.org> wrote:
>> >> >> > See attached.
>> >> >> >
>> >> >> > Returning a bool from main is a special case of return type
>> >> >> > mismatch. The
>> >> >> > common convention when returning a bool is that 'true' (== 1)
>> >> >> > indicates
>> >> >> > success and 'false' (== 0) failure. But since main expects a
>> >> >> > return
>> >> >> > value of
>> >> >> > 0 on success, returning a bool is usually unintended.
>> >> >>
>> >> >> I am not convinced that this is a high-value diagnostic. Returning
>> >> >> a
>> >> >> Boolean from main() may or may not be a bug (the returned value is
>> >> >> generally a convention more than anything else). Also, why Boolean
>> >> >> and
>> >> >> not, say, long long or float?
>> >> >
>> >> > I've seen this error often enough, but I think we need to be
>> >> > careful about false positives here. I recommend that we check only
>> >> > for explicit uses of boolean immediates (i.e. return true; or
>> >> > return false;) -- these are often bugs.
>> >>
>> >> I could get behind that.
>> >>
>> >> > Aaron, I disagree that the return value is just some kind of
>> >> > convention. It has a clear meaning.
>> >>
>> >> For many hosted environments, certainly. Freestanding
>> >> implementations?
>> >> Much less so, but I suppose this behavior is still reasonable enough
>> >> for them (not to mention, there may not even *be* a main() for a
>> >> freestanding implementation).
>> >>
>> >> > Furthermore, the behavior of the system can be quite different for
>> >> > a non-zero exit code than otherwise, and users who don't
>> >> > understand what's going on can find it very difficult to
>> >> > understand what's going wrong.
>> >>
>> >> That's a fair point, but my question still stands -- why only Boolean
>> >> values, and not "return 0x1234567800000000ULL;" or "return 1.2;"?
>> >>
>> >> Combining with your idea above, if the check flagged instances where
>> >> a
>> >> literal of non-integral type (other than Boolean) is returned from
>> >> main(), that seems like good value.
>> >
>> > Agreed.
>> >
>> > FWIW, if we have a function that returns 'int' and the user tries to
>> return '1.2' we should probably warn for any function.
>>
>> Good point, we already have -Wliteral-conversion (which catches 1.2)
>> and -Wconstant-conversion (which catches 0x1234567800000000ULL), and
>> -Wint-conversion for C programs returning awful things like string
>> literals, all of which are enabled by default. Perhaps Boolean values
>> really are the only case we don't explicitly warn about.
>>
>>
>> Wow, I'm amazed that we have no warning at all for converting, say,
>> 'true' to int (or indeed to float).
>>
>> Even with a warning for bool literal -> non-bool conversions in place, it
>> would still seem valuable to factor out a check for the corresponding case
>> in a return statement in main, since in that case we actually know what the
>> return value means, and the chance of a false positive goes way down.
>>
>> So I think that means we want two or possibly three checks here:
>>
>> 1) main returns bool literal
>> 2) -Wliteral-conversion warning for converting bool literal to another
>> type (excluding 1-bit unsigned integral bit-fields)
>> 3) and possibly: warning for implicit conversion of bool to
>> floating-point (new subgroup of -Wbool-conversion?)
>>
>> (ordered from most- to least-reasonable to enable by default).
>>
>> ~Aaron
>>
>> >
>> >  -Hal
>> >
>> >>
>> >> ~Aaron
>> >>
>> >> >
>> >> > Thanks again,
>> >> > Hal
>> >> >
>> >> >>
>> >> >> ~Aaron
>> >> >>
>> >> >> >
>> >> >> > _______________________________________________
>> >> >> > cfe-commits mailing list
>> >> >> > cfe-commits at lists.llvm.org
>> >> >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >> >> >
>> >> >> _______________________________________________
>> >> >> cfe-commits mailing list
>> >> >> cfe-commits at lists.llvm.org
>> >> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> >> >>
>> >> >
>> >> > --
>> >> > Hal Finkel
>> >> > Lead, Compiler Technology and Programming Languages
>> >> > Leadership Computing Facility
>> >> > Argonne National Laboratory
>> >>
>> >
>> > --
>> > Hal Finkel
>> > Lead, Compiler Technology and Programming Languages
>> > Leadership Computing Facility
>> > Argonne National Laboratory
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>>
>>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20161128/809bc748/attachment-0001.html>