Fwd: [C++ Patch] PR 83806 ("[6/7/8 Regression] Spurious -Wunused-but-set-parameter with nullptr")

Fwd: [C++ Patch] PR 83806 ("[6/7/8 Regression] Spurious -Wunused-but-set-parameter with nullptr")

[Paolo Carlini]

[-- Attachment #1: Type: text/plain, Size: 749 bytes --]

Hi,

this one should be rather straightforward. As noticed by Jakub, we 
started emitting the spurious warning with the fix for c++/69257, which, 
among other things, fixed decay_conversion wrt mark_rvalue_use and 
mark_lvalue_use calls. In particular it removed the mark_rvalue_use call 
at the very beginning of the function, thus now a PARM_DECL with 
NULLPTR_TYPE as type, being handled specially at the beginning of the 
function, doesn't get the mark_rvalue_use treatment - which, for 
example, POINTER_TYPE now gets later. I'm finishing testing on 
x86_64-linux the below. Ok if it passes?

Thanks, Paolo.

PS: sorry Jason, I have to re-send separately to the mailing list 
because some HTML crept in again. Grrr.

/////////////////////////


[-- Attachment #2: CL_83806 --]
[-- Type: text/plain, Size: 283 bytes --]

/cp
2018-02-08  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/83806
	* typeck.c (decay_conversion): Use mark_rvalue_use for the special
	case of nullptr too.

/testsuite
2018-02-08  Paolo Carlini  <paolo.carlini@oracle.com>

	PR c++/83806
	* g++.dg/warn/Wunused-parm-11.C: New.

[-- Attachment #3: patch_83806 --]
[-- Type: text/plain, Size: 1079 bytes --]

Index: testsuite/g++.dg/warn/Wunused-parm-11.C
===================================================================
--- testsuite/g++.dg/warn/Wunused-parm-11.C	(nonexistent)
+++ testsuite/g++.dg/warn/Wunused-parm-11.C	(working copy)
@@ -0,0 +1,13 @@
+// PR c++/83806
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wunused-but-set-parameter" }
+
+template <class X, class Y>
+bool equals(X x, Y y) {
+    return (x == y); 
+}
+
+int main() {
+    const char* p = nullptr;
+    equals(p, nullptr);
+}
Index: cp/typeck.c
===================================================================
--- cp/typeck.c	(revision 257477)
+++ cp/typeck.c	(working copy)
@@ -2009,7 +2009,10 @@ decay_conversion (tree exp,
     return error_mark_node;
 
   if (NULLPTR_TYPE_P (type) && !TREE_SIDE_EFFECTS (exp))
-    return nullptr_node;
+    {
+      exp = mark_rvalue_use (exp, loc, reject_builtin);
+      return nullptr_node;
+    }
 
   /* build_c_cast puts on a NOP_EXPR to make the result not an lvalue.
      Leave such NOP_EXPRs, since RHS is being used in non-lvalue context.  */

Re: [C++ Patch] PR 83806 ("[6/7/8 Regression] Spurious -Wunused-but-set-parameter with nullptr")

[Jason Merrill]

On Thu, Feb 8, 2018 at 6:22 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> this one should be rather straightforward. As noticed by Jakub, we started
> emitting the spurious warning with the fix for c++/69257, which, among other
> things, fixed decay_conversion wrt mark_rvalue_use and mark_lvalue_use
> calls. In particular it removed the mark_rvalue_use call at the very
> beginning of the function, thus now a PARM_DECL with NULLPTR_TYPE as type,
> being handled specially at the beginning of the function, doesn't get the
> mark_rvalue_use treatment - which, for example, POINTER_TYPE now gets later.
> I'm finishing testing on x86_64-linux the below. Ok if it passes?

A future -Wunused-but-set-variable might warn about the dead store to
exp; let's just discard the result of mark_rvalue_use.  OK with that
change.

> PS: sorry Jason, I have to re-send separately to the mailing list because
> some HTML crept in again. Grrr.

Hmm, I thought the mailing lists had been adjusted to allow some HTML.

Jason

Re: [C++ Patch] PR 83806 ("[6/7/8 Regression] Spurious -Wunused-but-set-parameter with nullptr")

[Paolo Carlini]

Hi,

On 08/02/2018 18:38, Jason Merrill wrote:
> On Thu, Feb 8, 2018 at 6:22 AM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
>> Hi,
>>
>> this one should be rather straightforward. As noticed by Jakub, we started
>> emitting the spurious warning with the fix for c++/69257, which, among other
>> things, fixed decay_conversion wrt mark_rvalue_use and mark_lvalue_use
>> calls. In particular it removed the mark_rvalue_use call at the very
>> beginning of the function, thus now a PARM_DECL with NULLPTR_TYPE as type,
>> being handled specially at the beginning of the function, doesn't get the
>> mark_rvalue_use treatment - which, for example, POINTER_TYPE now gets later.
>> I'm finishing testing on x86_64-linux the below. Ok if it passes?
> A future -Wunused-but-set-variable might warn about the dead store to
> exp; let's just discard the result of mark_rvalue_use.  OK with that
> change.
Agreed, thanks. By the way, maybe it's the right occasion to voice that 
I find myself often confused about this topic, that is which specific 
functions are modifying their arguments and there isn't a specific 
reason to assign the return value. I ask myself: why then we return 
something instead of void? Maybe just convenience while writing some 
expressions? Would make sense. Then, would it make sense to find a way 
to "mark" the functions which are modifying their arguments? Sometimes 
isn't immediately obvious because we are of course passing around 
pointers and using typedefs to obfuscate the whole thing. Maybe we 
should just put real C++ to good use ;)
>> PS: sorry Jason, I have to re-send separately to the mailing list because
>> some HTML crept in again. Grrr.
> Hmm, I thought the mailing lists had been adjusted to allow some HTML.
Yes, I noticed an exchange on gcc@ a while ago but didn't really follow 
the details. For your curiosity, the messages you got in your own 
mailbox definitely bounced with something like:

<gcc-patches@gcc.gnu.org>:
Invalid mime type "text/html" detected in message text or
attachment.  Please send plain text messages only.
Seehttp://sourceware.org/lists.html#sourceware-list-info  for more information.
Contactgcc-patches-owner@gcc.gnu.org  if you have questions about this. (#5.7.2)

Paolo.

Re: [C++ Patch] PR 83806 ("[6/7/8 Regression] Spurious -Wunused-but-set-parameter with nullptr")

[Jason Merrill]

On Thu, Feb 8, 2018 at 1:21 PM, Paolo Carlini <paolo.carlini@oracle.com> wrote:
> Hi,
>
> On 08/02/2018 18:38, Jason Merrill wrote:
>>
>> On Thu, Feb 8, 2018 at 6:22 AM, Paolo Carlini <paolo.carlini@oracle.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> this one should be rather straightforward. As noticed by Jakub, we
>>> started
>>> emitting the spurious warning with the fix for c++/69257, which, among
>>> other
>>> things, fixed decay_conversion wrt mark_rvalue_use and mark_lvalue_use
>>> calls. In particular it removed the mark_rvalue_use call at the very
>>> beginning of the function, thus now a PARM_DECL with NULLPTR_TYPE as
>>> type,
>>> being handled specially at the beginning of the function, doesn't get the
>>> mark_rvalue_use treatment - which, for example, POINTER_TYPE now gets
>>> later.
>>> I'm finishing testing on x86_64-linux the below. Ok if it passes?
>>
>> A future -Wunused-but-set-variable might warn about the dead store to
>> exp; let's just discard the result of mark_rvalue_use.  OK with that
>> change.
>
> Agreed, thanks. By the way, maybe it's the right occasion to voice that I
> find myself often confused about this topic, that is which specific
> functions are modifying their arguments and there isn't a specific reason to
> assign the return value. I ask myself: why then we return something instead
> of void? Maybe just convenience while writing some expressions? Would make
> sense. Then, would it make sense to find a way to "mark" the functions which
> are modifying their arguments? Sometimes isn't immediately obvious because
> we are of course passing around pointers and using typedefs to obfuscate the
> whole thing. Maybe we should just put real C++ to good use ;)

Well, usually we want to assign the result of mark_rvalue_use, it's
just that in this case we're returning nullptr_node instead of exp.

Jason