[C/C++] PR c++/66572. Fix Wlogical-op false positive

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?

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

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?