On Tuesday 30 of July 2013, Eli Friedman wrote:
> On Sun, Jul 28, 2013 at 3:41 AM, Lubos Lunak <[email protected]> wrote:
> > On Monday 01 of July 2013, Eli Friedman wrote:
> >> On Sat, Jun 29, 2013 at 11:23 PM, Lubos Lunak <[email protected]> wrote:
> >> > Hello,
> >> >
> >> > could somebody please review and commit the atached patch for
> >> > pr12463? Thank
> >> > you.
> >> >
> >> > + bool InMacro = LHS.get()->getLocStart().isMacroID() ||
> >>
> >> + RHS.get()->getLocStart().isMacroID();
> >>
> >> Why not just check the source location of the operator itself? It seems
> >> like we want to diagnose "MYMACRO1 == MYMACRO2".
> >
> > I don't know, because that's not my code. If you look at the patch, this
> > is just moved out of the first if(), in order to prevent it from applying
> > to all the cases. So maybe it's a good question, but it's not really part
> > of this issue.
>
> Okay, I'll put that aside, then.
Actually, looking at the end of clang/test/Sema/self-comparison.c , there is
an explicit check for not diagnosing MACRO1 == MACRO2, referencing
rdar://problem/8435950 as the reason, whatever that is.
> + !isa<StringLiteral>(LHSStripped) &&
> + !isa<ObjCEncodeExpr>(LHSStripped)) {
>
> A DeclRefExpr can't be a StringLiteral.
I guess that must be a left-over from updating the patch when it looked
bigger than it was because of whitespace changes.
> Why should array1 == array2 and array1 <= array2 behave differently
> inside macros?
> With this patch, we won't print the right diagnostic for array1 <
> array1 expanded from a macro.
The difference is that array1 <= array2 or array1 < array1 is nonsense, no
matter how I look at it, I can't imagine somebody writing that intentionally.
However array1 == array2 at least kind of may make sense, in code that e.g.
uses macros or #ifdef's (similarly to how some code may end up being
if(1==1) ).
But I have no strong opinion on this part, I can make it always or never warn
when macros are involved if you want (or if that rdar reference above has
anything to say about it).
Attached patch is just for string literals, which I hope is without any
problems or questions, and that's the original reason for my changes. I did
the arrays part just to keep the whole piece of code consistent, so I can
follow up with a patch for that, but I don't care much about that part.
--
Lubos Lunak
From 602f899c85f3359341e2dd891244dec9a3ba9780 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lubo=C5=A1=20Lu=C5=88=C3=A1k?= <[email protected]>
Date: Wed, 27 Nov 2013 17:36:30 +0100
Subject: [PATCH] warn about string literal comparisons even in macros
(pr12463)
They are unspecified even in macros.
---
lib/Sema/SemaExpr.cpp | 12 +++++++-----
test/Sema/exprs.c | 8 ++++++++
2 files changed, 15 insertions(+), 5 deletions(-)
diff --git a/lib/Sema/SemaExpr.cpp b/lib/Sema/SemaExpr.cpp
index 84487cc..3f77e83 100644
--- a/lib/Sema/SemaExpr.cpp
+++ b/lib/Sema/SemaExpr.cpp
@@ -7677,22 +7677,24 @@ QualType Sema::CheckCompareOperands(ExprResult &LHS, ExprResult &RHS,
if (!LHSType->hasFloatingRepresentation() &&
!(LHSType->isBlockPointerType() && IsRelational) &&
- !LHS.get()->getLocStart().isMacroID() &&
- !RHS.get()->getLocStart().isMacroID() &&
ActiveTemplateInstantiations.empty()) {
// For non-floating point types, check for self-comparisons of the form
// x == x, x != x, x < x, etc. These always evaluate to a constant, and
// often indicate logic errors in the program.
//
// NOTE: Don't warn about comparison expressions resulting from macro
- // expansion. Also don't warn about comparisons which are only self
+ // expansion, unless they do not make any sense at all.
+ // Also don't warn about comparisons which are only self
// comparisons within a template specialization. The warnings should catch
// obvious cases in the definition of the template anyways. The idea is to
// warn when the typed comparison operator will always evaluate to the same
// result.
+ bool InMacro = LHS.get()->getLocStart().isMacroID() ||
+ RHS.get()->getLocStart().isMacroID();
ValueDecl *DL = getCompareDecl(LHSStripped);
ValueDecl *DR = getCompareDecl(RHSStripped);
- if (DL && DR && DL == DR && !IsWithinTemplateSpecialization(DL)) {
+ if (DL && DR && DL == DR && !IsWithinTemplateSpecialization(DL) &&
+ !InMacro) {
DiagRuntimeBehavior(Loc, 0, PDiag(diag::warn_comparison_always)
<< 0 // self-
<< (Opc == BO_EQ
@@ -7700,7 +7702,7 @@ QualType Sema::CheckCompareOperands(ExprResult &LHS, ExprResult &RHS,
|| Opc == BO_GE));
} else if (DL && DR && LHSType->isArrayType() && RHSType->isArrayType() &&
!DL->getType()->isReferenceType() &&
- !DR->getType()->isReferenceType()) {
+ !DR->getType()->isReferenceType() && !InMacro) {
// what is it always going to eval to?
char always_evals_to;
switch(Opc) {
diff --git a/test/Sema/exprs.c b/test/Sema/exprs.c
index 2fb17e4..3e90146 100644
--- a/test/Sema/exprs.c
+++ b/test/Sema/exprs.c
@@ -125,6 +125,14 @@ int test12b(const char *X) {
return sizeof(X == "foo"); // no-warning
}
+// PR12463
+#define FOO_LITERAL "foo"
+#define STRING_LITERAL_COMPARE "a" == "b"
+int test12c(const char *X) {
+ return X == FOO_LITERAL; // expected-warning {{comparison against a string literal is unspecified (use strncmp instead)}}
+ return STRING_LITERAL_COMPARE; // expected-warning {{comparison against a string literal is unspecified (use strncmp instead)}}
+}
+
// rdar://6719156
void test13(
void (^P)()) { // expected-error {{blocks support disabled - compile with -fblocks}}
--
1.8.1.4
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
