Re: [PATCH] Warning for main returning a bool.
Thanks Richard for looking at the revised patch. On Mon, Nov 21, 2016 at 1:50 PM Richard Smith 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 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 > wrote: > > On Tue, Nov 15, 2016 at 2:55 PM, Aaron Ballman via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > > On Tue, Nov 15, 2016 at 5:44 PM, Hal Finkel wrote: > > - Original Message - > >> From: "Aaron Ballman" > >> To: "Hal Finkel" > >> Cc: "cfe-commits" , "Joshua Hurwitz" < > hurwi...@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 wrote: > >> > - Original Message - > >> >> From: "Aaron Ballman via cfe-commits" > >> >> To: "Joshua Hurwitz" > >> >> Cc: "cfe-commits" > >> >> 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 > >> >> 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 0x12345678ULL;" 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 0x12345678ULL), 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 convertin
Re: [PATCH] Warning for main returning a bool.
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 wrote: > On Tue, Nov 15, 2016 at 2:55 PM, Aaron Ballman via cfe-commits < > cfe-commits@lists.llvm.org> wrote: > > On Tue, Nov 15, 2016 at 5:44 PM, Hal Finkel wrote: > > - Original Message - > >> From: "Aaron Ballman" > >> To: "Hal Finkel" > >> Cc: "cfe-commits" , "Joshua Hurwitz" < > hurwi...@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 wrote: > >> > - Original Message - > >> >> From: "Aaron Ballman via cfe-commits" > >> >> To: "Joshua Hurwitz" > >> >> Cc: "cfe-commits" > >> >> 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 > >> >> 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 0x12345678ULL;" 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 0x12345678ULL), 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
Re: [PATCH] Warning for main returning a bool.
Friendly ping. Any further thoughts on this suggested warning? On Sat, Nov 5, 2016 at 1:48 PM Manuel Klimek wrote: > +richard > > On Fri, Oct 14, 2016 at 10:18 AM Joshua Hurwitz via cfe-commits < > cfe-commits@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. > > ___ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] Warning for main returning a bool.
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.
From 4b88e06e060fc70d6f82d17d92155377f939a96a Mon Sep 17 00:00:00 2001
From: Joshua Hurwitz
Date: Fri, 14 Oct 2016 13:04:26 -0400
Subject: [PATCH] Warning for main returning a bool.
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.
---
include/clang/Basic/DiagnosticSemaKinds.td | 2 ++
lib/Sema/SemaStmt.cpp | 5 +
test/Sema/warn-main-returns-bool.cpp | 17 +
3 files changed, 24 insertions(+)
create mode 100644 test/Sema/warn-main-returns-bool.cpp
diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 872311f..9b9115d 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -634,6 +634,8 @@ def warn_main_one_arg : Warning<"only one parameter on 'main' declaration">,
def err_main_arg_wrong : Error<"%select{first|second|third|fourth}0 "
"parameter of 'main' (%select{argument count|argument array|environment|"
"platform-specific data}0) must be of type %1">;
+def warn_main_returns_bool : Warning<"type of expression returned from 'main' "
+"(%0) should be 'int'">, InGroup;
def err_main_global_variable :
Error<"main cannot be declared as global variable">;
def warn_main_redefined : Warning<"variable named 'main' with external linkage "
diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp
index eba192d..dca81a2 100644
--- a/lib/Sema/SemaStmt.cpp
+++ b/lib/Sema/SemaStmt.cpp
@@ -3193,6 +3193,11 @@ StmtResult Sema::BuildReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp) {
if (FD->isNoReturn())
Diag(ReturnLoc, diag::warn_noreturn_function_has_return_expr)
<< FD->getDeclName();
+if (FD->isMain() && RetValExp)
+ if (Context.hasSameUnqualifiedType(RetValExp->getType(), Context.BoolTy))
+Diag(ReturnLoc, diag::warn_main_returns_bool)
+ << RetValExp->getType().getUnqualifiedType()
+ << RetValExp->getSourceRange();
} else if (ObjCMethodDecl *MD = getCurMethodDecl()) {
FnRetType = MD->getReturnType();
isObjCMethod = true;
diff --git a/test/Sema/warn-main-returns-bool.cpp b/test/Sema/warn-main-returns-bool.cpp
new file mode 100644
index 000..8763335
--- /dev/null
+++ b/test/Sema/warn-main-returns-bool.cpp
@@ -0,0 +1,17 @@
+// RUN: %clang_cc1 -std=c++11 -fsyntax-only -Wmain -verify %s
+
+// expected-note@+1 {{previous definition is here}}
+int main() {
+ return 0;
+} // no-warning
+
+// expected-error@+1 {{redefinition of 'main'}}
+int main() {
+ unsigned int u = 0;
+ return u;
+} // no-warning
+
+int main() {
+ const bool b = true;
+ return b; // expected-warning {{type of expression returned from 'main' ('bool') should be 'int'}}
+}
--
2.8.0.rc3.226.g39d4020
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
