Module Name: src
Committed By: rillig
Date: Mon Jan 4 23:47:27 UTC 2021
Modified Files:
src/tests/usr.bin/xlint/lint1: msg_169.c msg_169.exp
src/usr.bin/xlint/lint1: tree.c
Log Message:
lint: document and demonstrate the bug in check_precedence_confusion
It took quite a while to get to the correct interpretation of this small
piece of code and to draw the right conclusions from it. Now the bug is
finally ready to be fixed, as already announced in the test.
To generate a diff of this commit:
cvs rdiff -u -r1.3 -r1.4 src/tests/usr.bin/xlint/lint1/msg_169.c
cvs rdiff -u -r1.2 -r1.3 src/tests/usr.bin/xlint/lint1/msg_169.exp
cvs rdiff -u -r1.130 -r1.131 src/usr.bin/xlint/lint1/tree.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
Modified files:
Index: src/tests/usr.bin/xlint/lint1/msg_169.c
diff -u src/tests/usr.bin/xlint/lint1/msg_169.c:1.3 src/tests/usr.bin/xlint/lint1/msg_169.c:1.4
--- src/tests/usr.bin/xlint/lint1/msg_169.c:1.3 Mon Jan 4 22:41:56 2021
+++ src/tests/usr.bin/xlint/lint1/msg_169.c Mon Jan 4 23:47:27 2021
@@ -1,4 +1,4 @@
-/* $NetBSD: msg_169.c,v 1.3 2021/01/04 22:41:56 rillig Exp $ */
+/* $NetBSD: msg_169.c,v 1.4 2021/01/04 23:47:27 rillig Exp $ */
# 3 "msg_169.c"
// Test for message: precedence confusion possible: parenthesize! [169]
@@ -146,6 +146,36 @@ expected_precedence(int a, int b, int c)
ok = a + b * c;
}
-// TODO: add a test with unsigned long instead of unsigned, trying to
-// demonstrate that the typo in check_precedence_confusion actually has an
-// effect.
+/*
+ * Before tree.c 1.132 from 2021-01-04, there was a typo in
+ * check_precedence_confusion that prevented the right-hand operand from
+ * being flagged as possibly confusing if there was an implicit conversion
+ * or an explicit cast between the main operator ('|') and the nested
+ * operator ('&').
+ */
+void
+implicit_conversion_to_long(long la, int a)
+{
+ int ok;
+
+ ok = a & a | la; /* always marked as confusing */
+ ok = la | a & a; /* marked as confusing since tree.c 1.132 */
+
+ ok = (a & a) | la; /* always ok */
+ ok = la | (a & a); /* always ok */
+
+ /*
+ * Before tree.c 1.132, this expression didn't generate a warning
+ * because the right-hand operand was CVT, and there is no confusing
+ * precedence between BITOR and CVT.
+ *
+ * Since tree.c 1.132, this expression doesn't generate a warning
+ * because the right-hand operand is parenthesized. There is no way
+ * to have the right operand casted and at the same time not
+ * parenthesized since the cast operator has higher precedence.
+ *
+ * In summary, there is no visible change, but the implementation is
+ * now works as intended.
+ */
+ ok = la | (int)(a & a); /* always ok */
+}
Index: src/tests/usr.bin/xlint/lint1/msg_169.exp
diff -u src/tests/usr.bin/xlint/lint1/msg_169.exp:1.2 src/tests/usr.bin/xlint/lint1/msg_169.exp:1.3
--- src/tests/usr.bin/xlint/lint1/msg_169.exp:1.2 Mon Jan 4 15:52:51 2021
+++ src/tests/usr.bin/xlint/lint1/msg_169.exp Mon Jan 4 23:47:27 2021
@@ -22,3 +22,4 @@ msg_169.c(101): warning: precedence conf
msg_169.c(126): warning: precedence confusion possible: parenthesize! [169]
msg_169.c(127): warning: precedence confusion possible: parenthesize! [169]
msg_169.c(131): warning: precedence confusion possible: parenthesize! [169]
+msg_169.c(161): warning: precedence confusion possible: parenthesize! [169]
Index: src/usr.bin/xlint/lint1/tree.c
diff -u src/usr.bin/xlint/lint1/tree.c:1.130 src/usr.bin/xlint/lint1/tree.c:1.131
--- src/usr.bin/xlint/lint1/tree.c:1.130 Mon Jan 4 23:17:03 2021
+++ src/usr.bin/xlint/lint1/tree.c Mon Jan 4 23:47:26 2021
@@ -1,4 +1,4 @@
-/* $NetBSD: tree.c,v 1.130 2021/01/04 23:17:03 rillig Exp $ */
+/* $NetBSD: tree.c,v 1.131 2021/01/04 23:47:26 rillig Exp $ */
/*
* Copyright (c) 1994, 1995 Jochen Pohl
@@ -37,7 +37,7 @@
#include <sys/cdefs.h>
#if defined(__RCSID) && !defined(lint)
-__RCSID("$NetBSD: tree.c,v 1.130 2021/01/04 23:17:03 rillig Exp $");
+__RCSID("$NetBSD: tree.c,v 1.131 2021/01/04 23:47:26 rillig Exp $");
#endif
#include <float.h>
@@ -4011,28 +4011,11 @@ check_precedence_confusion(tnode_t *tn)
/*
* FIXME: There is a typo "tn->tn_op == CVT", which should
* rather be "rn->tn_op". Since tn must be a binary operator,
- * it can never be CVT.
+ * it can never be CVT, so the loop is never taken.
*
- * Before fixing this though, there should be a unit test
- * that demonstrates an actual change in behavior when this
- * bug gets fixed.
- *
- * rn must be a chain of casts and conversions, and at least
- * one of these must be a parenthesized cast.
- *
- * The argument of the innermost cast or conversion must not
- * be parenthesized.
- *
- * The argument of the innermost cast or conversion must be
- * an expression with confusing precedence. Since all these
- * expressions have lower precedence than a cast, these can
- * only appear as a parenthesized expression. This in turn
- * makes the whole loop superfluous.
- *
- * An edge case might be due to constant folding, if the
- * nodes created from constant folding did not preserve
- * tn_parenthesized properly. But that would be another bug,
- * so it doesn't count as an argument.
+ * Since the loop is never taken, if the right-hand operand
+ * is CVT, it is not followed to the actually interesting
+ * operator.
*/
for (rn = tn->tn_right; tn->tn_op == CVT; rn = rn->tn_left)
rparn |= rn->tn_parenthesized;
