Hey Aaron, I went ahead and made the two patches with the tests, let me know how they look! Is there anything I need to do to close D2506? http://llvm-reviews.chandlerc.com/D2507 http://llvm-reviews.chandlerc.com/D2508
Thanks. On Fri, Jan 3, 2014 at 12:33 PM, Aaron Ballman <[email protected]>wrote: > On Fri, Jan 3, 2014 at 12:25 PM, Michael Bao <[email protected]> wrote: > > In response to Bug 10642 in Bugzilla ( > http://llvm.org/bugs/show_bug.cgi?id=10642). I've checked this against > Revision 198291. > > > > # Removed the warning about a 'return' statement in a 'noreturn' > function if the 'return' statement is unreachable. > > # Warn users if they have a non-void return type on a 'noreturn' > function. > > > > This is my first patch, let me know if there's anything I need to do :) > Thanks! > > Thanks for your submission! > > You should reformat the patch to meet the coding guidelines: > http://www.llvm.org/docs/CodingStandards.html For instance, spaces > instead of tabs, no braces around single-line compound statements, > etc. > > Also, this should ideally be split into two patches since there are > two different things going on here. > > You should have test cases that demonstrate the behavioral changes for > both patches. > > > http://llvm-reviews.chandlerc.com/D2506 > > > > Files: > > lib/Sema/SemaStmt.cpp > > lib/Sema/SemaDecl.cpp > > include/clang/Basic/DiagnosticSemaKinds.td > > > > Index: lib/Sema/SemaStmt.cpp > > =================================================================== > > --- lib/Sema/SemaStmt.cpp > > +++ lib/Sema/SemaStmt.cpp > > @@ -2776,9 +2776,10 @@ > > QualType RelatedRetType; > > if (const FunctionDecl *FD = getCurFunctionDecl()) { > > FnRetType = FD->getResultType(); > > - if (FD->isNoReturn()) > > - Diag(ReturnLoc, diag::warn_noreturn_function_has_return_expr) > > - << FD->getDeclName(); > > + if (FD->isNoReturn()) { > > + DiagRuntimeBehavior(ReturnLoc, RetValExp, > > + > PDiag(diag::warn_noreturn_function_has_return_expr) << FD->getDeclName()); > > You can leave off the getDeclName here -- the diagnostics engine > understands anything that's a NamedDecl. > > > + } > > } else if (ObjCMethodDecl *MD = getCurMethodDecl()) { > > FnRetType = MD->getResultType(); > > if (MD->hasRelatedResultType() && MD->getClassInterface()) { > > Index: lib/Sema/SemaDecl.cpp > > =================================================================== > > --- lib/Sema/SemaDecl.cpp > > +++ lib/Sema/SemaDecl.cpp > > @@ -7392,6 +7392,10 @@ > > AddToScope = false; > > } > > > > + if (NewFD->isNoReturn() && > !NewFD->getResultType()->isVoidType()) { > > + Diag(NewFD->getLocation(), > diag::warn_noreturn_function_has_nonvoid_type) > > + << NewFD->getDeclName(); > > You can also leave off the getDeclName here. > > > + } > > return NewFD; > > } > > > > Index: include/clang/Basic/DiagnosticSemaKinds.td > > =================================================================== > > --- include/clang/Basic/DiagnosticSemaKinds.td > > +++ include/clang/Basic/DiagnosticSemaKinds.td > > @@ -6462,6 +6462,9 @@ > > def warn_falloff_noreturn_function : Warning< > > "function declared 'noreturn' should not return">, > > InGroup<InvalidNoreturn>; > > +def warn_noreturn_function_has_nonvoid_type : Warning< > > + "function %0 declared 'noreturn' should have not have a non-void > return type">, > > + InGroup<InvalidNoreturn>; > > This may be a bit more of a nitpick than is strictly necessary, but > 'noreturn' is the C++11 spelling, and _Noreturn is the C11 spelling -- > we should probably be a bit more distinct with the diagnostic for > clarity purposes. > > > def err_noreturn_block_has_return_expr : Error< > > "block declared 'noreturn' should not return">; > > def err_noreturn_missing_on_first_decl : Error< > > > > _______________________________________________ > > cfe-commits mailing list > > [email protected] > > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > > > Thanks! > > ~Aaron >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
