[PATCH 2/3] evaluate: warn on identical exprs around '||'

[PATCH 2/3] evaluate: warn on identical exprs around '||'

[Chris Forbes]

Same as prev, but for || operator.

Signed-off-by: Chris Forbes <chrisf@ijw.co.nz>
---
 evaluate.c                               |    4 ++++
 validation/check_identical_exprs_on_or.c |   24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)
 create mode 100644 validation/check_identical_exprs_on_or.c

diff --git a/evaluate.c b/evaluate.c
index 08f6ae6..11de7aa 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -985,6 +985,10 @@ static struct symbol *evaluate_logical(struct expression *expr)
 			expr_equiv(expr->left, expr->right)) {
 			warning(expr->pos, "identical expressions on both "
 				"sides of '&&'");
+		} else if (expr->op == SPECIAL_LOGICAL_OR &&
+			expr_equiv(expr->left, expr->right)) {
+			warning(expr->pos, "identical expressions on both "
+				"sides of '||'");
 		}
 	}
 
diff --git a/validation/check_identical_exprs_on_or.c b/validation/check_identical_exprs_on_or.c
new file mode 100644
index 0000000..74fe226
--- /dev/null
+++ b/validation/check_identical_exprs_on_or.c
@@ -0,0 +1,24 @@
+extern void bar(void);
+
+static void foo(void *a, void *b, void *c)
+{
+	if (a || a)	/* should warn */
+		bar();
+
+	if (a || b)	/* should not warn */
+		bar();
+
+	if ((a == b) || (a == b))	/* should warn */
+		bar();
+
+	if ((a == b) || (b == c))	/* should not warn */
+		bar();
+}
+/*
+ * check-name: A warning should be issued for identical expressions on both sides of the '||' operator.
+ *
+ * check-error-start
+check_identical_exprs_on_or.c:5:15: warning: identical expressions on both sides of '||'
+check_identical_exprs_on_or.c:11:22: warning: identical expressions on both sides of '||'
+ * check-error-end
+ */
-- 
1.7.4.1

[PATCH 1/3] evaluate: warn on identical exprs around '&&'

[Chris Forbes]

Adds a warning when identical expressions are found on both sides of the '&&' operator. This is a common error resulting from copy & paste.

Excludes identical expressions found while preprocessing, so we don't get upset about #if defined(FOO) && defined(BAR), which happens all the time, and is perfectly valid.

Signed-off-by: Chris Forbes <chrisf@ijw.co.nz>
---
 evaluate.c                                |  133 +++++++++++++++++++++++++++++
 validation/check_identical_exprs_on_and.c |   30 +++++++
 2 files changed, 163 insertions(+), 0 deletions(-)
 create mode 100644 validation/check_identical_exprs_on_and.c

diff --git a/evaluate.c b/evaluate.c
index f7a5678..4914442 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -853,8 +853,141 @@ static struct symbol *evaluate_conditional(struct expression *expr, int iterator
 	return ctype;
 }
 
+static int str_equal(struct string *a, struct string *b)
+{
+	return a->length == b->length &&
+		!memcmp(a->data, b->data, a->length);
+}
+
+static int ident_equal(struct ident *a, struct ident *b)
+{
+	/* only correct when used in the context of expr_equiv.
+		should compare symbols? for general case? */
+	return a->len == b->len &&
+		!memcmp(a->name, b->name, a->len);
+}
+
+/* expr_equiv and expr_list_equiv are mutually recursive */
+static int expr_equiv(struct expression *lhs, struct expression *rhs);
+
+static int expr_list_equiv(struct expression_list *lhs,
+			   struct expression_list *rhs)
+{
+	int l_length = expression_list_size(lhs);
+	int r_length = expression_list_size(rhs);
+	struct expression **l_items, **r_items;
+	int i;
+
+	if (l_length != r_length)
+		return 0;
+
+	l_items = alloca(sizeof( *l_items ) * l_length);
+	r_items = alloca(sizeof( *r_items ) * r_length);
+
+	linearize_ptr_list((struct ptr_list *)lhs, (void **)l_items, l_length);
+	linearize_ptr_list((struct ptr_list *)rhs, (void **)r_items, r_length);
+
+	for (i = 0; i < l_length; i++) {
+		if (!expr_equiv(l_items[i], r_items[i]))
+			return 0;
+	}
+
+	return 1;
+}
+
+int expr_equiv(struct expression *lhs, struct expression *rhs)
+{
+	/* recursively determine if lhs ~ rhs. */
+	if (!lhs || !rhs) return 0;
+
+	if (lhs->type != rhs->type) return 0;
+	if (lhs->flags != rhs->flags) return 0;
+	if (lhs->op != rhs->op) return 0;
+	if (lhs->ctype != rhs->ctype) return 0;
+
+	switch (lhs->type) {
+	case EXPR_VALUE:
+		return lhs->value == rhs->value &&
+			lhs->taint == rhs->taint;
+	case EXPR_FVALUE:
+		return lhs->fvalue == rhs->fvalue;
+	case EXPR_STRING:
+		return lhs->wide == rhs->wide &&
+			str_equal(lhs->string, rhs->string);
+/*	case EXPR_UNOP:	*/	/* this is mentioned in the union, but doesn't
+				actually exist */
+	case EXPR_PREOP:
+	case EXPR_POSTOP:
+		return lhs->op_value == rhs->op_value &&
+			expr_equiv(lhs->unop, rhs->unop);
+	case EXPR_SYMBOL:
+	case EXPR_TYPE:
+		return lhs->symbol == rhs->symbol;
+	case EXPR_BINOP:
+	case EXPR_COMMA:
+	case EXPR_COMPARE:
+	case EXPR_LOGICAL:
+	case EXPR_ASSIGNMENT:
+		return expr_equiv(lhs->left, rhs->left) &&
+			expr_equiv(lhs->right, rhs->right);
+	case EXPR_DEREF:
+		return expr_equiv(lhs->deref, rhs->deref) &&
+			ident_equal(lhs->member, rhs->member);
+	case EXPR_SLICE:
+		return expr_equiv(lhs->base, rhs->base) &&
+			lhs->r_bitpos == rhs->r_bitpos &&
+			lhs->r_nrbits == rhs->r_nrbits;
+	case EXPR_CAST:
+	case EXPR_SIZEOF:
+		return lhs->cast_type == rhs->cast_type &&
+			expr_equiv(lhs->cast_expression,
+				rhs->cast_expression);
+	case EXPR_CONDITIONAL:
+	case EXPR_SELECT:
+		return expr_equiv(lhs->conditional, rhs->conditional) &&
+			expr_equiv(lhs->cond_true, rhs->cond_true) &&
+			expr_equiv(lhs->cond_false, rhs->cond_false);
+	case EXPR_CALL:
+		return expr_equiv(lhs->fn, rhs->fn) &&
+			expr_list_equiv(lhs->args, rhs->args);
+	case EXPR_LABEL:
+		return lhs->label_symbol == rhs->label_symbol;
+	case EXPR_INITIALIZER:
+		return expr_list_equiv(lhs->expr_list, rhs->expr_list);
+	case EXPR_IDENTIFIER:
+		return ident_equal(lhs->expr_ident, rhs->expr_ident) &&
+			lhs->field == rhs->field &&
+			expr_equiv(lhs->ident_expression,
+				rhs->ident_expression);
+	case EXPR_INDEX:
+		return lhs->idx_from == rhs->idx_from &&
+			lhs->idx_to == rhs->idx_to &&
+			expr_equiv(lhs->idx_expression, rhs->idx_expression);
+	case EXPR_POS:
+		return lhs->init_offset == rhs->init_offset &&
+			lhs->init_nr == rhs->init_nr &&
+			expr_equiv(lhs->init_expr, rhs->init_expr);
+	case EXPR_OFFSETOF:
+		return lhs->in == rhs->in &&
+			expr_equiv(lhs->down, rhs->down);
+
+	default:
+		return 0;
+	}
+
+	return 1;
+}
+
 static struct symbol *evaluate_logical(struct expression *expr)
 {
+	if (!preprocessing) {
+		if (expr->op == SPECIAL_LOGICAL_AND &&
+			expr_equiv(expr->left, expr->right)) {
+			warning(expr->pos, "identical expressions on both "
+				"sides of '&&'");
+		}
+	}
+
 	if (!evaluate_conditional(expr->left, 0))
 		return NULL;
 	if (!evaluate_conditional(expr->right, 0))
diff --git a/validation/check_identical_exprs_on_and.c b/validation/check_identical_exprs_on_and.c
new file mode 100644
index 0000000..6560429
--- /dev/null
+++ b/validation/check_identical_exprs_on_and.c
@@ -0,0 +1,30 @@
+extern void bar(void);
+
+static void foo(void *a, void *b, void *c)
+{
+	if (a && a)	/* should warn */
+		bar();
+
+	if (a && b)	/* should not warn */
+		bar();
+
+	if ((a == b) && (a == b))	/* should warn */
+		bar();
+
+	if ((a == b) && (b == c))	/* should not warn */
+		bar();
+}
+
+/* should not warn */
+#if defined(BLETCHEROUS_PLATFORM) && defined(BROKEN_VERSION_OF_SAME)
+	/* do something suitably bizarre */
+#endif
+
+/*
+ * check-name: A warning should be issued for identical expressions on both sides of the '&&' operator.
+ *
+ * check-error-start
+check_identical_exprs_on_and.c:5:15: warning: identical expressions on both sides of '&&'
+check_identical_exprs_on_and.c:11:22: warning: identical expressions on both sides of '&&'
+ * check-error-end
+ */
-- 
1.7.4.1

[PATCH 2/3] evaluate: warn on identical exprs around '||'

[Chris Forbes]

Same as prev, but for || operator.

Signed-off-by: Chris Forbes <chrisf@ijw.co.nz>
---
 evaluate.c                               |    4 ++++
 validation/check_identical_exprs_on_or.c |   24 ++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 0 deletions(-)
 create mode 100644 validation/check_identical_exprs_on_or.c

diff --git a/evaluate.c b/evaluate.c
index 4914442..0e5d691 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -985,6 +985,10 @@ static struct symbol *evaluate_logical(struct expression *expr)
 			expr_equiv(expr->left, expr->right)) {
 			warning(expr->pos, "identical expressions on both "
 				"sides of '&&'");
+		} else if (expr->op == SPECIAL_LOGICAL_OR &&
+			expr_equiv(expr->left, expr->right)) {
+			warning(expr->pos, "identical expressions on both "
+				"sides of '||'");
 		}
 	}
 
diff --git a/validation/check_identical_exprs_on_or.c b/validation/check_identical_exprs_on_or.c
new file mode 100644
index 0000000..74fe226
--- /dev/null
+++ b/validation/check_identical_exprs_on_or.c
@@ -0,0 +1,24 @@
+extern void bar(void);
+
+static void foo(void *a, void *b, void *c)
+{
+	if (a || a)	/* should warn */
+		bar();
+
+	if (a || b)	/* should not warn */
+		bar();
+
+	if ((a == b) || (a == b))	/* should warn */
+		bar();
+
+	if ((a == b) || (b == c))	/* should not warn */
+		bar();
+}
+/*
+ * check-name: A warning should be issued for identical expressions on both sides of the '||' operator.
+ *
+ * check-error-start
+check_identical_exprs_on_or.c:5:15: warning: identical expressions on both sides of '||'
+check_identical_exprs_on_or.c:11:22: warning: identical expressions on both sides of '||'
+ * check-error-end
+ */
-- 
1.7.4.1

[PATCH 3/3] evaluate: warn on identical exprs on ?:

[Chris Forbes]

Adds a warning when identical expressions are found on both the true and false branches of ?:. This is another common copy-paste error.

Signed-off-by: Chris Forbes <chrisf@ijw.co.nz>
---
 evaluate.c                                 |    6 ++++++
 validation/check_identical_exprs_on_cond.c |   13 +++++++++++++
 2 files changed, 19 insertions(+), 0 deletions(-)
 create mode 100644 validation/check_identical_exprs_on_cond.c

diff --git a/evaluate.c b/evaluate.c
index 0e5d691..c339e63 100644
--- a/evaluate.c
+++ b/evaluate.c
@@ -1220,6 +1220,12 @@ static struct symbol *evaluate_conditional_expression(struct expression *expr)
 	const char * typediff;
 	int qual;
 
+	if (!preprocessing) {
+		if (expr_equiv(expr->cond_true, expr->cond_false))
+			warning(expr->pos, "identical expressions on both "
+				"branches of '?:'");
+	}
+
 	if (!evaluate_conditional(expr->conditional, 0))
 		return NULL;
 	if (!evaluate_expression(expr->cond_false))
diff --git a/validation/check_identical_exprs_on_cond.c b/validation/check_identical_exprs_on_cond.c
new file mode 100644
index 0000000..85a9938
--- /dev/null
+++ b/validation/check_identical_exprs_on_cond.c
@@ -0,0 +1,13 @@
+extern void bar(void);
+
+static void foo(void *a, void *b, void *c)
+{
+	void * d = a ? b : b; /* should warn */
+}
+/*
+ * check-name: A warning should be issued for identical expressions on both the true and false branches of the '?:' operator.
+ *
+ * check-error-start
+check_identical_exprs_on_cond.c:5:22: warning: identical expressions on both branches of '?:'
+ * check-error-end
+ */
-- 
1.7.4.1

Re: [PATCH 1/3] evaluate: warn on identical exprs around '&&'

[Jonathan Neuschäfer]

On Sun, Aug 28, 2011 at 03:14:18PM +1200, Chris Forbes wrote:
> +	case EXPR_BINOP:
> +	case EXPR_COMMA:
> +	case EXPR_COMPARE:
> +	case EXPR_LOGICAL:
> +	case EXPR_ASSIGNMENT:
> +		return expr_equiv(lhs->left, rhs->left) &&
> +			expr_equiv(lhs->right, rhs->right);
[...]
> +	if ((a == b) && (a == b))	/* should warn */
> +		bar();
> +
> +	if ((a == b) && (b == c))	/* should not warn */
> +		bar();

Should it maybe also handle cases like this?

if ((a == b) && (b == a))
	bar();

Thanks,
	Jonathan Neuschäfer
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] evaluate: warn on identical exprs around '&&'

[Josh Triplett]

On Mon, Aug 29, 2011 at 12:01:06PM +0200, Jonathan Neuschäfer wrote:
> On Sun, Aug 28, 2011 at 03:14:18PM +1200, Chris Forbes wrote:
> > +	case EXPR_BINOP:
> > +	case EXPR_COMMA:
> > +	case EXPR_COMPARE:
> > +	case EXPR_LOGICAL:
> > +	case EXPR_ASSIGNMENT:
> > +		return expr_equiv(lhs->left, rhs->left) &&
> > +			expr_equiv(lhs->right, rhs->right);
> [...]
> > +	if ((a == b) && (a == b))	/* should warn */
> > +		bar();
> > +
> > +	if ((a == b) && (b == c))	/* should not warn */
> > +		bar();
> 
> Should it maybe also handle cases like this?
> 
> if ((a == b) && (b == a))
> 	bar();

That seems both significantly harder to handle and significantly less
likely (since it won't occur due to simple copy/paste).  Probably worth
doing at some point, but I don't think a first pass needs to consider
it.

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html