* [C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive
@ 2015-06-20 0:23 Mikhail Maltsev
2015-06-23 19:59 ` Marek Polacek
0 siblings, 1 reply; 7+ messages in thread
From: Mikhail Maltsev @ 2015-06-20 0:23 UTC (permalink / raw)
To: gcc-patches; +Cc: Marek Polacek, Jason Merrill
[-- Attachment #1: Type: text/plain, Size: 336 bytes --]
Hi.
In current implementation we avoid giving "logical and/or of equal
expressions" warning for literal constant operands. The attached patch
fixes the check to make it treat const-qualified VAR_DECLs with constant
initializers the same way.
Bootstrapped and regtested on x86_64-linux. OK for trunk?
--
Regards,
Mikhail Maltsev
[-- Attachment #2: pr66572.clog --]
[-- Type: text/plain, Size: 370 bytes --]
gcc/c-family/ChangeLog:
2015-06-19 Mikhail Maltsev <maltsevm@gmail.com>
PR c++/66572
* c-common.c (warn_logical_operator): Treat constant-initialized
VAR_DECLs like literal constants.
gcc/testsuite/ChangeLog:
2015-06-19 Mikhail Maltsev <maltsevm@gmail.com>
PR c++/66572
* c-c++-common/Wlogical-op-2.c: New test.
* g++.dg/warn/Wlogical-op-2.C: New test.
[-- Attachment #3: pr66572.patch --]
[-- Type: text/x-patch, Size: 3170 bytes --]
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
index dc2bf00..38c7be9 100644
--- a/gcc/c-family/c-common.c
+++ b/gcc/c-family/c-common.c
@@ -1766,9 +1766,12 @@ warn_logical_operator (location_t location, enum tree_code code, tree type,
return;
}
- /* 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;
/* This warning only makes sense with logical operands. */
diff --git a/gcc/testsuite/c-c++-common/Wlogical-op-2.c b/gcc/testsuite/c-c++-common/Wlogical-op-2.c
new file mode 100644
index 0000000..47f5c28
--- /dev/null
+++ b/gcc/testsuite/c-c++-common/Wlogical-op-2.c
@@ -0,0 +1,24 @@
+/* PR c++/66572 */
+/* { dg-do compile } */
+/* { dg-options "-Wlogical-op" } */
+
+#ifndef __cplusplus
+# define bool _Bool
+# define true 1
+# define false 0
+#endif
+
+void
+no_warn ()
+{
+ const bool cst_a = true;
+ const bool cst_b = false;
+
+ if (cst_a || cst_b) {}
+ if (cst_a && cst_b) {}
+ if (true && cst_a) {}
+ if (true || cst_a) {}
+ if (false && cst_a) {}
+ if (false || cst_a) {}
+}
+
diff --git a/gcc/testsuite/g++.dg/warn/Wlogical-op-2.C b/gcc/testsuite/g++.dg/warn/Wlogical-op-2.C
new file mode 100644
index 0000000..252592c6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/warn/Wlogical-op-2.C
@@ -0,0 +1,59 @@
+// PR c++/66572
+// { dg-do compile }
+// { dg-options "-Wlogical-op" }
+
+#if __cplusplus >= 201103L
+# define CONSTEXPR constexpr
+#else
+# define CONSTEXPR const
+#endif
+
+struct true_type
+{
+ static CONSTEXPR bool value = true;
+};
+
+struct false_type
+{
+ static CONSTEXPR bool value = true;
+};
+
+template<typename T>
+struct is_unsigned : false_type { };
+
+template<>
+struct is_unsigned<unsigned int> : true_type { };
+
+template<typename T1, typename T2>
+bool both_are_unsigned ()
+{
+ return is_unsigned <T1>::value && is_unsigned <T2>::value;
+}
+
+template<typename T1, typename T2>
+bool one_of_is_unsigned ()
+{
+ return is_unsigned <T1>::value || is_unsigned <T2>::value;
+}
+
+void
+foo ()
+{
+ both_are_unsigned <unsigned int, unsigned int> ();
+ both_are_unsigned <int, unsigned int> ();
+ both_are_unsigned <int, int> ();
+ one_of_is_unsigned <unsigned int, unsigned int> ();
+ one_of_is_unsigned <int, unsigned int> ();
+ one_of_is_unsigned <int, int> ();
+}
+
+void
+bar (const int parm_a)
+{
+ const bool a = parm_a;
+ if (a && a) {} /* { dg-warning "logical .and. of equal expressions" } */
+ if (a || a) {} /* { dg-warning "logical .or. of equal expressions" } */
+ if (parm_a && parm_a) {} /* { dg-warning "logical .and. of equal expressions" } */
+ if (parm_a || parm_a) {} /* { dg-warning "logical .or. of equal expressions" } */
+}
+
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive
2015-06-20 0:23 [C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive Mikhail Maltsev
@ 2015-06-23 19:59 ` Marek Polacek
2015-06-24 14:54 ` Mikhail Maltsev
0 siblings, 1 reply; 7+ messages in thread
From: Marek Polacek @ 2015-06-23 19:59 UTC (permalink / raw)
To: Mikhail Maltsev; +Cc: gcc-patches, Jason Merrill
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.
foo<unsigned, signed>();
in main().
Marek
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive
2015-06-23 19:59 ` Marek Polacek
@ 2015-06-24 14:54 ` Mikhail Maltsev
2015-07-14 16:43 ` Marek Polacek
0 siblings, 1 reply; 7+ messages in thread
From: Mikhail Maltsev @ 2015-06-24 14:54 UTC (permalink / raw)
To: Marek Polacek; +Cc: gcc-patches, Jason Merrill
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.
> foo<unsigned, signed>();
> in main().
Maybe my snippet does not express clearly enough what it was supposed to
express. I actually meant something like this:
template<class _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::pair<T, T>
object from an std::pair<U, 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive
2015-06-24 14:54 ` Mikhail Maltsev
@ 2015-07-14 16:43 ` Marek Polacek
2015-07-15 8:01 ` Mikhail Maltsev
2015-07-21 11:14 ` Marek Polacek
0 siblings, 2 replies; 7+ messages in thread
From: Marek Polacek @ 2015-07-14 16:43 UTC (permalink / raw)
To: Mikhail Maltsev; +Cc: gcc-patches, Jason Merrill
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:
>
> template<class _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::pair<T, T>
> object from an std::pair<U, 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 <polacek@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;
+};
+
+template<typename T>
+struct is_unsigned : false_type {};
+
+template<>
+struct is_unsigned<unsigned> : true_type {};
+
+template<typename T1, typename T2>
+bool foo()
+{
+ return is_unsigned<T1>::value && is_unsigned<T2>::value;
+}
+
+int main()
+{
+ foo<unsigned, unsigned>();
+}
Marek
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive
2015-07-14 16:43 ` Marek Polacek
@ 2015-07-15 8:01 ` Mikhail Maltsev
2015-07-21 11:14 ` Marek Polacek
1 sibling, 0 replies; 7+ messages in thread
From: Mikhail Maltsev @ 2015-07-15 8:01 UTC (permalink / raw)
To: Marek Polacek; +Cc: gcc-patches, Jason Merrill
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive
2015-07-14 16:43 ` Marek Polacek
2015-07-15 8:01 ` Mikhail Maltsev
@ 2015-07-21 11:14 ` Marek Polacek
2015-07-23 18:22 ` Jeff Law
1 sibling, 1 reply; 7+ messages in thread
From: Marek Polacek @ 2015-07-21 11:14 UTC (permalink / raw)
To: Jason Merrill; +Cc: gcc-patches
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 <polacek@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;
> +};
> +
> +template<typename T>
> +struct is_unsigned : false_type {};
> +
> +template<>
> +struct is_unsigned<unsigned> : true_type {};
> +
> +template<typename T1, typename T2>
> +bool foo()
> +{
> + return is_unsigned<T1>::value && is_unsigned<T2>::value;
> +}
> +
> +int main()
> +{
> + foo<unsigned, unsigned>();
> +}
>
> Marek
Marek
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive
2015-07-21 11:14 ` Marek Polacek
@ 2015-07-23 18:22 ` Jeff Law
0 siblings, 0 replies; 7+ messages in thread
From: Jeff Law @ 2015-07-23 18:22 UTC (permalink / raw)
To: Marek Polacek, Jason Merrill; +Cc: gcc-patches
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 <polacek@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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-07-23 18:04 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-20 0:23 [C/C++ PATCH] PR c++/66572. Fix Wlogical-op false positive Mikhail Maltsev
2015-06-23 19:59 ` Marek Polacek
2015-06-24 14:54 ` Mikhail Maltsev
2015-07-14 16:43 ` Marek Polacek
2015-07-15 8:01 ` Mikhail Maltsev
2015-07-21 11:14 ` Marek Polacek
2015-07-23 18:22 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).