[cfe-commits] [LLVMdev] clang errors on void main()

[David Blaikie]

On Mon, Feb 13, 2012 at 3:55 AM, Vasiliy Korchagin <korchagin at ispras.ru> wrote:
> 13.02.2012 12:28, Vasiliy Korchagin пишет:
>>
>> 13.02.2012 05:39, Chandler Carruth пишет:
>>>
>>> On Sun, Feb 12, 2012 at 6:17 AM, Vasiliy Korchagin <korchagin at ispras.ru
>>> <mailto:korchagin at ispras.ru>> wrote:
>>>
>>> On 12.02.2012 07:39, Chandler Carruth wrote:
>>>>
>>>> On Sat, Feb 11, 2012 at 5:31 AM, Vasiliy Korchagin
>>>> <korchagin at ispras.ru <mailto:korchagin at ispras.ru>> wrote:
>>>>
>>>> I agree, without setting implicit return zero bit changes in
>>>> codegen are not necessary. New version of patch is attached.
>>>>
>>>>
>>>> The warning you're adding, as David suggested, should be under a
>>>> separate flag.
>>>>
>>>> It should also be an extwarn as this is technically a language
>>>> extension, and it should be enabled by default (you may already
>>>> have that, just want it clarified).
>>>>
>>>> --- a/lib/Sema/SemaDecl.cpp
>>>> +++ b/lib/Sema/SemaDecl.cpp
>>>> @@ -5795,8 +5795,13 @@ void Sema::CheckMain(FunctionDecl* FD,
>>>> const DeclSpec& DS) {
>>>> const FunctionType* FT = T->getAs<FunctionType>();
>>>> if (!Context.hasSameUnqualifiedType(FT->getResultType(),
>>>> Context.IntTy)) {
>>>> - Diag(FD->getTypeSpecStartLoc(), diag::err_main_returns_nonint);
>>>> - FD->setInvalidDecl(true);
>>>> + if (getLangOptions().C99) {
>>>> + // In C we allow main() to have non-integer return type.
>>>> + Diag(FD->getTypeSpecStartLoc(),
>>>> diag::warn_main_returns_nonint);
>>>>
>>>> If you want this to be enabled in all C modes, you should accept
>>>> more than just C99: "!getLangOptions().CPlusPlus" is a good
>>>> candidate here.
>>>>
>>>> @@ -7204,7 +7209,8 @@ Decl *Sema::ActOnFinishFunctionBody(Decl
>>>> *dcl, Stmt *Body,
>>>> if (FD->isMain()) {
>>>> // C and C++ allow for main to automagically return 0.
>>>> // Implements C++ [basic.start.main]p5 and C99 5.1.2.2.3.
>>>> - FD->setHasImplicitReturnZero(true);
>>>> + if (!getLangOptions().C99)
>>>> + FD->setHasImplicitReturnZero(true);
>>>>
>>>> This isn't correct. You need to be checking for a non-int return
>>>> type here, not for C99. If the function has an int return type we
>>>> want the implicit return zero.
>>>
>>> Now new warning is under the separate flag ("-Wmain-return-type")
>>> and it is an extwarn and it is enabled by default. Also problems
>>> with C modes and implicit return zero are fixed. New patch is
>>> attached.
>>>
>>>
>>> This is super close. Two minor nits, and it should be good-to-go:
>>>
>>> 1) The canonical prefix for an ExtWarn diagnostic is "ext_", not "warn_".
>>>
>>> 2) I would name the test something more generic so we can fold more main
>>> tests into this file. 'main.c' would be fine.
>>
>> Warning prefix is changed and test is renamed.
>>
>> Vasiliy Korchagin,
>> The Institute for System Programming of the Russian Academy of Sciences
>>
>>
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
> Sorry, last time patch was incorrect. Now it is ok.

This looks good to me (hopefully someone else can sign off on it for
checkin). The only optional thoughts I have (you can probably wait
until sign off to see if someone agrees with me/thinks it's worth
changing):

* the warning could be grouped under the 'main' warning group (for
some back-compat with GCC)
* if (FD->getResultType()->isIntegerType())
FD->setHasImplicitReturnZero(true); could be written as
FD->setHasImplicitReturnZero(FD->getResultType()->isIntegerType()) -
that might not be an improvement in readability, though.
(or, honestly, setHasImplicitReturnZero could have that check
internally & either assert that it has int return so you couldn't've
made the bug you did previously - or it could just silently no-op when
you have a non-integer return type... no big deal, though)