On Mon, Feb 13, 2012 at 3:55 AM, Vasiliy Korchagin <[email protected]> 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 <[email protected] >>> <mailto:[email protected]>> wrote: >>> >>> On 12.02.2012 07:39, Chandler Carruth wrote: >>>> >>>> On Sat, Feb 11, 2012 at 5:31 AM, Vasiliy Korchagin >>>> <[email protected] <mailto:[email protected]>> 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 >> [email protected] >> 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) _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
