Without HasSideEffects you get lots of warnings in templates. From what i
remember there were some discussion about not warning in templates but i might
remember wrong, it's been a while now.
I have removed HasSideEffects now and modified the testfiles that started to
trigg on the warning.
Also added extra parens to silence the warning.
//Anders
________________________________________
Från: Jordan Rose [[email protected]]
Skickat: den 13 maj 2014 18:38
Till: Anders Rönnholm
Cc: [email protected]; Daniel Marjamäki
Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
Sorry for letting this slip through the cracks! I know it's now been a month
and a half, but what were the false positives you saw without the
HasSideEffects check? For example:
+int SizeofFunctionCallExpression() {
+ return sizeof(SizeofDefine() - 1);
+} // no-warning
This should have a warning, since the function is not called. If it interferes
with the VLA thing Aaron brought up, though...
I never got a response to this:
+ if (Binop->getLHS()->getType()->isArrayType() ||
+ Binop->getLHS()->getType()->isAnyPointerType() ||
+ Binop->getRHS()->getType()->isArrayType() ||
+ Binop->getRHS()->getType()->isAnyPointerType())
+ return;
I don't think this is correct...the user is only trying to get ptrdiff_t if
both the LHS and RHS are pointer-ish.
Finally, how about using an extra set of parens to silence the warning? It's
harder to typo, and we have some precedent for that.
Jordan
On May 13, 2014, at 3:27 , Anders Rönnholm
<[email protected]<mailto:[email protected]>> wrote:
Pinging
________________________________________
Från: Anders Rönnholm
Skickat: den 27 mars 2014 11:09
Till: Jordan Rose
Cc: [email protected]<mailto:[email protected]>; Daniel Marjamäki
Ämne: SV: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
New patch with new diagnostic message. I couldn't come up with a better wording
so i'm using your suggestion. I don't know of a good way to silence the warning.
I removed the check for HasSideEffects previously but had to take back. I
noticed that the patch triggered some false positives without it.
//Anders
.......................................................................................................................
Anders Rönnholm Senior Engineer
Evidente ES East AB Warfvinges väg 34 SE-112 51 Stockholm Sweden
Mobile: +46 (0)70 912 42 54
E-mail:
[email protected]<mailto:[email protected]>
www.evidente.se<http://www.evidente.se>
________________________________________
Från: Jordan Rose [[email protected]]
Skickat: den 31 januari 2014 18:50
Till: Anders Rönnholm
Cc: [email protected]; Daniel Marjamäki
Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
Sorry to have let this slip! This is looking good, but I had one more thought
about the diagnostic message. It says "may yield unexpected results", but
doesn't really explain what those unexpected results are. I was wondering if we
could work the type into the message for the operator case.
"operand of sizeof is a binary expression of type %0, which may not be intended"
I don't like that wording either, but at least this one makes people say "what?
why isn't it [the type I actually want]?". Also, should there be a way to
silence the warning?
What do you think?
Jordan
On Jan 23, 2014, at 6:40 , Anders Rönnholm
<[email protected]<mailto:[email protected]>> wrote:
Hi,
New one with comments handled.
________________________________________
Från: Jordan Rose [[email protected]<mailto:[email protected]>]
Skickat: den 20 december 2013 19:15
Till: Anders Rönnholm
Cc: [email protected]<mailto:[email protected]>; Daniel Marjamäki;
Anna Zaks; David Blaikie; Richard Smith; Matt Calabrese
Ämne: Re: [PATCH] [StaticAnalyzer] New checker Sizeof on expression
On Dec 10, 2013, at 4:38 , Anders Rönnholm
<[email protected]<mailto:[email protected]><mailto:[email protected]>>
wrote:
Are you OK to commit this patch or do you see more issues?
I'm not sure if anyone else has ideological concerns. There's always a flag to
turn this off, I suppose.
+ if (S.isSFINAEContext())
+ return;
Code style: extra indent?
+ if(E->HasSideEffects(S.getASTContext()))
+ return;
sizeof doesn't evaluate its argument, so I'm not sure why you wouldn't want to
warn here.
+ const FunctionDecl *FD = S.getCurFunctionDecl();
+ if(FD && FD->isFunctionTemplateSpecialization())
+ return;
Code style: space after if. (Above too, actually.)
+ if (Binop->getLHS()->getType()->isArrayType() ||
+ Binop->getLHS()->getType()->isAnyPointerType() ||
+ Binop->getRHS()->getType()->isArrayType() ||
+ Binop->getRHS()->getType()->isAnyPointerType())
+ return;
I don't think this is correct...the user is only trying to get ptrdiff_t if
both the LHS and RHS are pointer-ish.
+def warn_sizeof_bin_op : Warning<
+ "using sizeof() on an expression with an operator may yield unexpected
results">,
+ InGroup<SizeofOnExpression>;
+
+def warn_sizeof_sizeof : Warning<
+ "using sizeof() on sizeof() may yield unexpected results.">,
+ InGroup<SizeofOnExpression>;
+
sizeof doesn't actually require parens, so we shouldn't put the parens in the
diagnostics.
<sizeofonexpression.diff>
Index: test/Sema/warn-sizeof-array-decay.c
===================================================================
--- test/Sema/warn-sizeof-array-decay.c (revision 208561)
+++ test/Sema/warn-sizeof-array-decay.c (working copy)
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-sizeof-on-expression -verify %s
void f(int x) {
char foo[10];
Index: test/Sema/exprs.c
===================================================================
--- test/Sema/exprs.c (revision 208561)
+++ test/Sema/exprs.c (working copy)
@@ -159,7 +159,8 @@
x /= 0; // expected-warning {{division by zero is undefined}}
x %= 0; // expected-warning {{remainder by zero is undefined}}
- x = sizeof(x/0); // no warning.
+ x = sizeof(x/0); // expected-warning{{operand of sizeof is a binary expression of type 'int', which may not be intended}}
+ // expected-note@-1 {{use extra parentheses to silence this warning}}
}
// PR6501 & PR11857
Index: test/Sema/sizeofonexpression.c
===================================================================
--- test/Sema/sizeofonexpression.c (revision 0)
+++ test/Sema/sizeofonexpression.c (revision 0)
@@ -0,0 +1,53 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+#define DEF (1+2)
+
+struct s {
+ int xx;
+ int yy;
+};
+
+int SizeofStructExpression(struct s p) {
+ return sizeof(sizeof(p) + 1); // expected-warning{{operand of sizeof is a binary expression of type 'unsigned long', which may not be intended}}
+} // expected-note@-1 {{use extra parentheses to silence this warning}}
+
+int SizeofStructMemberExpression(struct s p) {
+ return sizeof(p.xx * p.yy); // expected-warning{{operand of sizeof is a binary expression of type 'int', which may not be intended}}
+} // expected-note@-1 {{use extra parentheses to silence this warning}}
+
+int SizeofLiteralExpression() {
+ return sizeof(2 + 1); // expected-warning{{operand of sizeof is a binary expression of type 'int', which may not be intended}}
+} // expected-note@-1 {{use extra parentheses to silence this warning}}
+
+int SizeofRefExpression() {
+ int x = 1, y = 2;
+ return sizeof(x / y); // expected-warning{{operand of sizeof is a binary expression of type 'int', which may not be intended}}
+} // expected-note@-1 {{use extra parentheses to silence this warning}}
+
+int SizeofDefine() {
+ return sizeof(DEF);
+} // no-warning
+
+int SizeofDefineExpression() {
+ return sizeof(DEF + 2);
+} // no-warning
+
+int SizeofDefineExpression2() {
+ return sizeof(2 - DEF);
+} // no-warning
+
+int SizeofFunctionCallExpression() {
+ return sizeof(SizeofDefine() - 1); // expected-warning{{operand of sizeof is a binary expression of type 'int', which may not be intended}}
+} // expected-note@-1 {{use extra parentheses to silence this warning}}
+
+int SizeofSizeof() {
+ return sizeof(sizeof(1)); // expected-warning{{using sizeof on sizeof may yield unexpected results}}
+} // expected-note@-1 {{use extra parentheses to silence this warning}}
+
+int SizeofComparison() {
+ return sizeof(1+1) == 1; // expected-warning{{operand of sizeof is a binary expression of type 'int', which may not be intended}}
+} // expected-note@-1 {{use extra parentheses to silence this warning}}
+
+int SizeofComparisonSilence() {
+ return sizeof((1+1)) == 1;
+}
Index: test/CXX/expr/expr.prim/expr.prim.general/p12-0x.cpp
===================================================================
--- test/CXX/expr/expr.prim/expr.prim.general/p12-0x.cpp (revision 208561)
+++ test/CXX/expr/expr.prim/expr.prim.general/p12-0x.cpp (working copy)
@@ -9,9 +9,9 @@
};
int i = sizeof(S::m); // ok
-int j = sizeof(S::m + 42); // ok
+int j = sizeof(S::m + 42); // expected-warning{{operand of sizeof is a binary expression of type 'int *', which may not be intended}}
+ // expected-note@-1 {{use extra parentheses to silence this warning}}
-
struct T {
int n;
static void f() {
Index: test/CXX/temp/temp.decls/temp.variadic/injected-class-name.cpp
===================================================================
--- test/CXX/temp/temp.decls/temp.variadic/injected-class-name.cpp (revision 208561)
+++ test/CXX/temp/temp.decls/temp.variadic/injected-class-name.cpp (working copy)
@@ -1,5 +1,4 @@
// RUN: %clang_cc1 -std=c++11 -fsyntax-only -verify %s
-// expected-no-diagnostics
// Check for declaration matching with out-of-line declarations and
// variadic templates, which involves proper computation of the
@@ -64,13 +63,14 @@
template<typename R, typename ...ArgTypes>
struct X1<R(ArgTypes...)> {
- unsigned_c<sizeof(1 + g(ArgTypes()...))> f();
- };
+ unsigned_c<sizeof(1 + g(ArgTypes()...))> f(); // expected-warning{{operand of sizeof is a binary expression of type 'int', which may not be intended}}
+ }; // expected-note@-1 {{use extra parentheses to silence this warning}}
+
template<typename R, typename ...ArgTypes>
- unsigned_c<sizeof(1 + g(ArgTypes()...))> X1<R(ArgTypes...)>::f() {
+ unsigned_c<sizeof(1 + g(ArgTypes()...))> X1<R(ArgTypes...)>::f() {
return unsigned_c<sizeof(int)>();
}
- X1<int(float, double)> xif2;
+ X1<int(float, double)> xif2; // expected-note {{in instantiation of template class 'rdar8848837::X1<int (float, double)>' requested here}}
}
Index: test/SemaCXX/decl-expr-ambiguity.cpp
===================================================================
--- test/SemaCXX/decl-expr-ambiguity.cpp (revision 208561)
+++ test/SemaCXX/decl-expr-ambiguity.cpp (working copy)
@@ -14,7 +14,8 @@
void(a), ++a;
if (int(a)+1) {}
for (int(a)+1;;) {} // expected-warning {{expression result unused}}
- a = sizeof(int()+1);
+ a = sizeof(int()+1);// expected-warning{{operand of sizeof is a binary expression of type 'int', which may not be intended}}
+ // expected-note@-1 {{use extra parentheses to silence this warning}}
a = sizeof(int(1));
typeof(int()+1) a2; // expected-error {{extension used}}
(int(1)); // expected-warning {{expression result unused}}
Index: test/SemaCXX/enable_if.cpp
===================================================================
--- test/SemaCXX/enable_if.cpp (revision 208561)
+++ test/SemaCXX/enable_if.cpp (working copy)
@@ -49,7 +49,7 @@
int not_constexpr();
template<int N> void valuedep() __attribute__((enable_if(N == not_constexpr(), "")));
-template <typename T> void instantiationdep() __attribute__((enable_if(sizeof(sizeof(T)) != 0, "")));
+template <typename T> void instantiationdep() __attribute__((enable_if(sizeof(T) != 0, "")));
void test() {
X x;
Index: test/SemaCXX/sizeofonexpression.cpp
===================================================================
--- test/SemaCXX/sizeofonexpression.cpp (revision 0)
+++ test/SemaCXX/sizeofonexpression.cpp (revision 0)
@@ -0,0 +1,31 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+template<typename T, typename RESULT,
+ typename ARG1, typename ARG2, typename ARG3>
+class DoesDebug
+{
+ template <typename U, RESULT (U::*)(ARG1, ARG2, ARG3)> struct Check;
+ template <typename U> static char checkFn(Check<U, &U::Debug> *);
+ template <typename U> static int checkFn(...);
+public:
+ enum { Exist = sizeof(checkFn<T>(0)) == sizeof(char) }; // no-warning
+
+ int binOp() {
+ return sizeof(checkFn<T>(0)+1);
+ } // no-warning
+
+ int sizeofsizeof() {
+ return sizeof(sizeof(checkFn<T>(0))); // expected-warning{{using sizeof on sizeof may yield unexpected results}}
+ } // expected-note@-1 {{use extra parentheses to silence this warning}}
+};
+class A {
+public:
+ A(): a(1) { }
+ A(int p) : a(sizeof(p + 1)) { } // expected-warning{{operand of sizeof is a binary expression of type 'int', which may not be intended}}
+ int a; // expected-note@-1 {{use extra parentheses to silence this warning}}
+};
+
+void f()
+{
+ int bb = 0;
+ int b = sizeof(bb + 1); // expected-warning{{operand of sizeof is a binary expression of type 'int', which may not be intended}}
+} // expected-note@-1 {{use extra parentheses to silence this warning}}
Index: test/SemaTemplate/instantiate-var-template.cpp
===================================================================
--- test/SemaTemplate/instantiate-var-template.cpp (revision 208561)
+++ test/SemaTemplate/instantiate-var-template.cpp (working copy)
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 -verify -std=c++1y %s
+// RUN: %clang_cc1 -Wno-sizeof-on-expression -verify -std=c++1y %s
namespace PR17846 {
template <typename T> constexpr T pi = T(3.14);
Index: test/SemaTemplate/instantiate-sizeof.cpp
===================================================================
--- test/SemaTemplate/instantiate-sizeof.cpp (revision 208561)
+++ test/SemaTemplate/instantiate-sizeof.cpp (working copy)
@@ -1,11 +1,10 @@
// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
-// expected-no-diagnostics
// Make sure we handle contexts correctly with sizeof
template<typename T> void f(T n) {
int buffer[n];
- [] { int x = sizeof(sizeof(buffer)); }();
-}
+ [] { int x = sizeof(sizeof(buffer)); }(); // expected-warning{{using sizeof on sizeof may yield unexpected results}}
+} // expected-note@-1 {{use extra parentheses to silence this warning}}
int main() {
f<int>(1);
}
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td (revision 208561)
+++ include/clang/Basic/DiagnosticGroups.td (working copy)
@@ -284,6 +284,7 @@
def : DiagGroup<"synth">;
def SizeofArrayArgument : DiagGroup<"sizeof-array-argument">;
def SizeofArrayDecay : DiagGroup<"sizeof-array-decay">;
+def SizeofOnExpression : DiagGroup<"sizeof-on-expression">;
def SizeofPointerMemaccess : DiagGroup<"sizeof-pointer-memaccess">;
def StaticInInline : DiagGroup<"static-in-inline">;
def StaticLocalInInline : DiagGroup<"static-local-in-inline">;
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td (revision 208561)
+++ include/clang/Basic/DiagnosticSemaKinds.td (working copy)
@@ -4475,6 +4475,15 @@
"sizeof on pointer operation will return size of %0 instead of %1">,
InGroup<SizeofArrayDecay>;
+def warn_sizeof_bin_op : Warning<
+ "operand of sizeof is a binary expression of type %0, which may not be intended">,
+ InGroup<SizeofOnExpression>;
+def warn_sizeof_sizeof : Warning<
+ "using sizeof on sizeof may yield unexpected results">,
+ InGroup<SizeofOnExpression>;
+def note_warn_sizeof_silence : Note<
+ "use extra parentheses to silence this warning">;
+
def err_sizeof_nonfragile_interface : Error<
"application of '%select{alignof|sizeof}1' to interface %0 is "
"not supported on this architecture and platform">;
Index: lib/Sema/SemaExpr.cpp
===================================================================
--- lib/Sema/SemaExpr.cpp (revision 208561)
+++ lib/Sema/SemaExpr.cpp (working copy)
@@ -3317,6 +3317,62 @@
<< ICE->getSubExpr()->getType();
}
+/// \brief Warns if sizeof expression has a binary operation or contains a
+/// sizeof.
+static void warnOnSizeofOnExpression(Sema &S, const Expr *E) {
+ if (S.isSFINAEContext())
+ return;
+
+ const FunctionDecl *FD = S.getCurFunctionDecl();
+ if (FD && FD->isFunctionTemplateSpecialization())
+ return;
+
+ if (const ParenExpr *PE = dyn_cast<ParenExpr>(E)) {
+ if (const BinaryOperator *Binop =
+ dyn_cast<BinaryOperator>(PE->getSubExpr())) {
+ if (!Binop->isMultiplicativeOp() &&
+ !Binop->isAdditiveOp())
+ return;
+
+ if ((Binop->getLHS()->getType()->isArrayType() ||
+ Binop->getLHS()->getType()->isAnyPointerType()) &&
+ (Binop->getRHS()->getType()->isArrayType() ||
+ Binop->getRHS()->getType()->isAnyPointerType()))
+ return;
+
+ // Don't warn if expression is based on macro.
+ if ((Binop->getLHS()->getExprLoc().isMacroID()) ||
+ (Binop->getRHS()->getExprLoc().isMacroID()))
+ return;
+
+ SourceRange DiagRange(Binop->getLHS()->getLocStart(),
+ Binop->getRHS()->getLocEnd());
+ S.Diag(Binop->getOperatorLoc(), diag::warn_sizeof_bin_op) << DiagRange <<
+ Binop->getType();
+
+ SourceLocation EndLoc = S.PP.getLocForEndOfToken(Binop->getRHS()->getLocEnd());
+ S.Diag(Binop->getOperatorLoc(), diag::note_warn_sizeof_silence)
+ << FixItHint::CreateInsertion(Binop->getLHS()->getLocStart(), "&")
+ << FixItHint::CreateReplacement(SourceRange(Binop->getOperatorLoc()), "[")
+ << FixItHint::CreateInsertion(EndLoc, "]");
+ } else if (const UnaryExprOrTypeTraitExpr *UnaryExpr =
+ dyn_cast<UnaryExprOrTypeTraitExpr>(PE->getSubExpr())) {
+ if (UnaryExpr->getKind() != UETT_SizeOf)
+ return;
+
+ SourceRange DiagRange(UnaryExpr->getLocStart(),
+ UnaryExpr->getLocEnd());
+ S.Diag(UnaryExpr->getOperatorLoc(), diag::warn_sizeof_sizeof) << DiagRange;
+
+ SourceLocation EndLoc = S.PP.getLocForEndOfToken(UnaryExpr->getLocEnd());
+ S.Diag(UnaryExpr->getOperatorLoc(), diag::note_warn_sizeof_silence)
+ << FixItHint::CreateInsertion(UnaryExpr->getLocStart(), "&")
+ << FixItHint::CreateReplacement(SourceRange(UnaryExpr->getOperatorLoc()), "[")
+ << FixItHint::CreateInsertion(EndLoc, "]");
+ }
+ }
+}
+
/// \brief Check the constraints on expression operands to unary type expression
/// and type traits.
///
@@ -3379,6 +3435,7 @@
warnOnSizeofOnArrayDecay(*this, BO->getOperatorLoc(), BO->getType(),
BO->getRHS());
}
+ warnOnSizeofOnExpression(*this, E);
}
return false;
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
