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 <rich...@metafoo.co.uk> 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 <hfin...@anl.gov> wrote:
> > ----- Original Message -----
> >> From: "Aaron Ballman" <aa...@aaronballman.com>
> >> To: "Hal Finkel" <hfin...@anl.gov>
> >> Cc: "cfe-commits" <cfe-commits@lists.llvm.org>, "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 <hfin...@anl.gov> wrote:
> >> > ----- Original Message -----
> >> >> From: "Aaron Ballman via cfe-commits" <cfe-commits@lists.llvm.org>
> >> >> To: "Joshua Hurwitz" <hurwi...@google.com>
> >> >> Cc: "cfe-commits" <cfe-commits@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@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@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
> >> >>
> >> >
> >> > --
> >> > 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@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
From 4af80a1791b13aa513ca7b63cab13fe84310d604 Mon Sep 17 00:00:00 2001
From: Joshua Hurwitz <hurwi...@google.com>
Date: Mon, 21 Nov 2016 10:10:26 -0500
Subject: [PATCH] Warning for main returning a bool literal.
Returning a bool literal 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 literal is usually
unintended.
---
include/clang/Basic/DiagnosticSemaKinds.td | 2 ++
lib/Sema/SemaStmt.cpp | 4 ++++
test/Sema/warn-main-returns-bool-literal.cpp | 20 ++++++++++++++++++++
3 files changed, 26 insertions(+)
create mode 100644 test/Sema/warn-main-returns-bool-literal.cpp
diff --git a/include/clang/Basic/DiagnosticSemaKinds.td b/include/clang/Basic/DiagnosticSemaKinds.td
index 0b45911..ac78b89 100644
--- a/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/include/clang/Basic/DiagnosticSemaKinds.td
@@ -646,6 +646,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_literal : Warning<"bool literal returned from "
+ "'main'">, InGroup<Main>;
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..79ad049 100644
--- a/lib/Sema/SemaStmt.cpp
+++ b/lib/Sema/SemaStmt.cpp
@@ -3193,6 +3193,10 @@ 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 (isa<CXXBoolLiteralExpr>(RetValExp))
+ Diag(ReturnLoc, diag::warn_main_returns_bool_literal)
+ << RetValExp->getSourceRange();
} else if (ObjCMethodDecl *MD = getCurMethodDecl()) {
FnRetType = MD->getReturnType();
isObjCMethod = true;
diff --git a/test/Sema/warn-main-returns-bool-literal.cpp b/test/Sema/warn-main-returns-bool-literal.cpp
new file mode 100644
index 0000000..188f161
--- /dev/null
+++ b/test/Sema/warn-main-returns-bool-literal.cpp
@@ -0,0 +1,20 @@
+// 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() {
+ return 1.0;
+} // no-warning
+
+int main() {
+ bool b = true;
+ return b; // no-warning
+}
+
+int main() {
+ return true; // expected-warning {{bool literal returned from 'main'}}
+}
--
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
