Re: [C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive
On 07/21/2015 04:56 AM, Marek Polacek wrote: Ping. On Tue, Jul 14, 2015 at 06:38:12PM +0200, Marek Polacek wrote: Ok, in that case I think easiest would the following (I hit the same issue when writing the -Wtautological-compare patch): Bootstrapped/regtested on x86_64-linux, ok for trunk? 2015-07-14 Marek Polacek pola...@redhat.com PR c++/66572 * pt.c (tsubst_copy_and_build): Add warn_logical_op sentinel. * g++.dg/warn/Wlogical-op-2.C: New test. I realize it's C++, but it's simple enough for me. Ok for the trunk. jeff
Re: [C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive
Ping.
On Tue, Jul 14, 2015 at 06:38:12PM +0200, Marek Polacek wrote:
Ok, in that case I think easiest would the following (I hit the same issue
when writing the -Wtautological-compare patch):
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2015-07-14 Marek Polacek pola...@redhat.com
PR c++/66572
* pt.c (tsubst_copy_and_build): Add warn_logical_op sentinel.
* g++.dg/warn/Wlogical-op-2.C: New test.
diff --git gcc/cp/pt.c gcc/cp/pt.c
index 2097963..0c9712a 100644
--- gcc/cp/pt.c
+++ gcc/cp/pt.c
@@ -14893,6 +14893,7 @@ tsubst_copy_and_build (tree t,
{
warning_sentinel s1(warn_type_limits);
warning_sentinel s2(warn_div_by_zero);
+ warning_sentinel s3(warn_logical_op);
tree op0 = RECUR (TREE_OPERAND (t, 0));
tree op1 = RECUR (TREE_OPERAND (t, 1));
tree r = build_x_binary_op
diff --git gcc/testsuite/g++.dg/warn/Wlogical-op-2.C
gcc/testsuite/g++.dg/warn/Wlogical-op-2.C
index e69de29..755db08 100644
--- gcc/testsuite/g++.dg/warn/Wlogical-op-2.C
+++ gcc/testsuite/g++.dg/warn/Wlogical-op-2.C
@@ -0,0 +1,30 @@
+// PR c++/66572
+// { dg-do compile { target c++11 } }
+// { dg-options -Wlogical-op }
+
+struct false_type
+{
+static constexpr bool value = false;
+};
+
+struct true_type
+{
+static constexpr bool value = true;
+};
+
+templatetypename T
+struct is_unsigned : false_type {};
+
+template
+struct is_unsignedunsigned : true_type {};
+
+templatetypename T1, typename T2
+bool foo()
+{
+return is_unsignedT1::value is_unsignedT2::value;
+}
+
+int main()
+{
+foounsigned, unsigned();
+}
Marek
Marek
Re: [C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive
On 07/14/2015 07:38 PM, Marek Polacek wrote: Ok, in that case I think easiest would the following (I hit the same issue when writing the -Wtautological-compare patch): Thanks for taking care of this issue. -- Regards, Mikhail Maltsev
Re: [C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive
Sorry it's taken so long to respond.
On Wed, Jun 24, 2015 at 05:43:05PM +0300, Mikhail Maltsev wrote:
That looks wrong, because with TREE_CONSTANT we'd warn in C but not in C++
for the following:
const int a = 4;
void
f (void)
{
const int b = 4;
static const int c = 5;
if (a a) {}
if (b b) {}
if (c c) {}
}
Actually for this case the patch silences the warning both for C and
C++. It's interesting that Clang warns like this:
That's really not what I see. With your patch, cc1 warns on that testcase
while cc1plus does not.
test.c:7:10: warning: use of logical '' with constant operand
[-Wconstant-logical-operand]
It does not warn for my testcase with templates. It also does not warn
about:
void
bar (const int parm_a)
{
const bool a = parm_a;
if (a a) {}
if (a || a) {}
if (parm_a parm_a) {}
if (parm_a || parm_a) {}
}
I think we want 4 warnings here, but vanilla cc1 only issues 2 warnings.
Maybe my snippet does not express clearly enough what it was supposed to
express. I actually meant something like this:
templateclass _U1, class _U2, class = typename
enable_if__and_is_convertible_U1, _T1,
is_convertible_U2, _T2::value::type
constexpr pair(pair_U1, _U2 __p)
: first(std::forward_U1(__p.first)),
second(std::forward_U2(__p.second)) { }
(it's std::pair move constructor)
It is perfectly possible that the user will construct an std::pairT, T
object from an std::pairU, U. In this case we get an and of two
identical is_convertible instantiations. The difference is that here
there is a clever __and_ template which helps to avoid warnings. Well,
at least I now know a good way to suppress them in my code :).
Though I still think that this warning is bogus. Probably the correct
(and the hard) way to check templates is to compare ASTs of the operands
before any substitutions.
But for now I could try to implement an idea, which I mentioned in the
bug report: add a new flag to enum tsubst_flags, and set it when we
check ASTs which depend on parameters of a template being instantiated
(we already have similar checks for macro expansions). What do you think
about such approach?
Ok, in that case I think easiest would the following (I hit the same issue
when writing the -Wtautological-compare patch):
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2015-07-14 Marek Polacek pola...@redhat.com
PR c++/66572
* pt.c (tsubst_copy_and_build): Add warn_logical_op sentinel.
* g++.dg/warn/Wlogical-op-2.C: New test.
diff --git gcc/cp/pt.c gcc/cp/pt.c
index 2097963..0c9712a 100644
--- gcc/cp/pt.c
+++ gcc/cp/pt.c
@@ -14893,6 +14893,7 @@ tsubst_copy_and_build (tree t,
{
warning_sentinel s1(warn_type_limits);
warning_sentinel s2(warn_div_by_zero);
+ warning_sentinel s3(warn_logical_op);
tree op0 = RECUR (TREE_OPERAND (t, 0));
tree op1 = RECUR (TREE_OPERAND (t, 1));
tree r = build_x_binary_op
diff --git gcc/testsuite/g++.dg/warn/Wlogical-op-2.C
gcc/testsuite/g++.dg/warn/Wlogical-op-2.C
index e69de29..755db08 100644
--- gcc/testsuite/g++.dg/warn/Wlogical-op-2.C
+++ gcc/testsuite/g++.dg/warn/Wlogical-op-2.C
@@ -0,0 +1,30 @@
+// PR c++/66572
+// { dg-do compile { target c++11 } }
+// { dg-options -Wlogical-op }
+
+struct false_type
+{
+static constexpr bool value = false;
+};
+
+struct true_type
+{
+static constexpr bool value = true;
+};
+
+templatetypename T
+struct is_unsigned : false_type {};
+
+template
+struct is_unsignedunsigned : true_type {};
+
+templatetypename T1, typename T2
+bool foo()
+{
+return is_unsignedT1::value is_unsignedT2::value;
+}
+
+int main()
+{
+foounsigned, unsigned();
+}
Marek
Re: [C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive
On 23.06.2015 22:49, Marek Polacek wrote:
On Sat, Jun 20, 2015 at 03:02:06AM +0300, Mikhail Maltsev wrote:
- /* We do not warn for constants because they are typical of macro
- expansions that test for features. */
- if (CONSTANT_CLASS_P (op_left) || CONSTANT_CLASS_P (op_right))
+ /* We do not warn for literal constants because they are typical of macro
+ expansions that test for features. Likewise, we do not warn for
+ const-qualified and constexpr variables which are initialized by
constant
+ expressions, because they can come from e.g. type_traits or similar
user
+ code. */
+ if (TREE_CONSTANT (op_left) || TREE_CONSTANT (op_right))
return;
That looks wrong, because with TREE_CONSTANT we'd warn in C but not in C++
for the following:
const int a = 4;
void
f (void)
{
const int b = 4;
static const int c = 5;
if (a a) {}
if (b b) {}
if (c c) {}
}
Actually for this case the patch silences the warning both for C and
C++. It's interesting that Clang warns like this:
test.c:7:10: warning: use of logical '' with constant operand
[-Wconstant-logical-operand]
It does not warn for my testcase with templates. It also does not warn
about:
void
bar (const int parm_a)
{
const bool a = parm_a;
if (a a) {}
if (a || a) {}
if (parm_a parm_a) {}
if (parm_a || parm_a) {}
}
EDG does not give any warnings at all (in all 3 testcases).
Note that const-qualified types are checked using TYPE_READONLY.
Yes, but I think we should warn about const-qualified types like in the
example above (and in your recent patch).
But I'm not even sure that the warning in the original testcase in the PR
is bogus; you won't get any warning when using e.g.
foounsigned, signed();
in main().
Maybe my snippet does not express clearly enough what it was supposed to
express. I actually meant something like this:
templateclass _U1, class _U2, class = typename
enable_if__and_is_convertible_U1, _T1,
is_convertible_U2, _T2::value::type
constexpr pair(pair_U1, _U2 __p)
: first(std::forward_U1(__p.first)),
second(std::forward_U2(__p.second)) { }
(it's std::pair move constructor)
It is perfectly possible that the user will construct an std::pairT, T
object from an std::pairU, U. In this case we get an and of two
identical is_convertible instantiations. The difference is that here
there is a clever __and_ template which helps to avoid warnings. Well,
at least I now know a good way to suppress them in my code :).
Though I still think that this warning is bogus. Probably the correct
(and the hard) way to check templates is to compare ASTs of the operands
before any substitutions.
But for now I could try to implement an idea, which I mentioned in the
bug report: add a new flag to enum tsubst_flags, and set it when we
check ASTs which depend on parameters of a template being instantiated
(we already have similar checks for macro expansions). What do you think
about such approach?
--
Regards,
Mikhail Maltsev
Re: [C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive
On Sat, Jun 20, 2015 at 03:02:06AM +0300, Mikhail Maltsev wrote:
- /* We do not warn for constants because they are typical of macro
- expansions that test for features. */
- if (CONSTANT_CLASS_P (op_left) || CONSTANT_CLASS_P (op_right))
+ /* We do not warn for literal constants because they are typical of macro
+ expansions that test for features. Likewise, we do not warn for
+ const-qualified and constexpr variables which are initialized by
constant
+ expressions, because they can come from e.g. type_traits or similar
user
+ code. */
+ if (TREE_CONSTANT (op_left) || TREE_CONSTANT (op_right))
return;
That looks wrong, because with TREE_CONSTANT we'd warn in C but not in C++
for the following:
const int a = 4;
void
f (void)
{
const int b = 4;
static const int c = 5;
if (a a) {}
if (b b) {}
if (c c) {}
}
Note that const-qualified types are checked using TYPE_READONLY.
But I'm not even sure that the warning in the original testcase in the PR
is bogus; you won't get any warning when using e.g.
foounsigned, signed();
in main().
Marek
