| Message ID | 215a033c-9cb1-5ddd-56b9-389c1ddb1ae5@oracle.com |
|---|---|
| State | New |
| Headers | show |
| Series | [C++] PR 83806 ("[6/7/8 Regression] Spurious -Wunused-but-set-parameter with nullptr") | expand |
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
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.
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
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. */