From: David Malcolm <dmalcolm@redhat.com>
To: Jason Merrill <jason@redhat.com>
Cc: Jeff Law <law@redhat.com>, gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] v5: C++: more location wrapper nodes (PR c++/43064, PR c++/43486)
Date: Mon, 17 Dec 2018 23:30:00 -0000 [thread overview]
Message-ID: <1545089414.4619.239.camel@redhat.com> (raw)
In-Reply-To: <4a0448bb-8f3e-92cb-0e13-f8928632cc0c@redhat.com>
On Mon, 2018-12-17 at 14:33 -0500, Jason Merrill wrote:
> On 12/14/18 7:17 PM, David Malcolm wrote:
> > + /* Since default args are effectively part of the function
> > type,
> > + strip location wrappers here, since otherwise the
> > location of
> > + one function's default arguments is arbitrarily chosen
> > for
> > + all functions with similar signature (due to
> > canonicalization
> > + of function types). */
>
> Hmm, looking at this again, why would this happen? I see that
> type_list_equal uses == to compare default arguments, so two
> function
> types with the same default argument but different location wrappers
> shouldn't be combined.
>
> Jason
Thanks.
I did some digging into this. I added this strip to fix
g++.dg/template/defarg6.C
but it looks like I was overzealous (the comment is correct, but it's
papering over a problem).
It turns out that type_list_equal is doing more than just pointer
equality; it's hitting the simple_cst_equal part of the && at line
7071:
7063 bool
7064 type_list_equal (const_tree l1, const_tree l2)
7065 {
7066 const_tree t1, t2;
7067
7068 for (t1 = l1, t2 = l2; t1 && t2; t1 = TREE_CHAIN (t1), t2 = TREE_CHAIN (t2))
7069 if (TREE_VALUE (t1) != TREE_VALUE (t2)
7070 || (TREE_PURPOSE (t1) != TREE_PURPOSE (t2)
7071 && ! (1 == simple_cst_equal (TREE_PURPOSE (t1), TREE_PURPOSE (t2))
7072 && (TREE_TYPE (TREE_PURPOSE (t1))
7073 == TREE_TYPE (TREE_PURPOSE (t2))))))
7074 return false;
7075
7076 return t1 == t2;
7077 }
What's happening is that there are two different functions with
identical types apart from the locations of their (equal) default
arguments: both of the TREE_PURPOSEs are NON_LVALUE_EXPR wrappers
around a CONST_DECL enum value (at different source locations).
simple_cst_equal is stripping the location wrappers here:
7311 if (CONVERT_EXPR_CODE_P (code1) || code1 == NON_LVALUE_EXPR)
7312 {
7313 if (CONVERT_EXPR_CODE_P (code2)
7314 || code2 == NON_LVALUE_EXPR)
7315 return simple_cst_equal (TREE_OPERAND (t1, 0), TREE_OPERAND (t2, 0));
7316 else
7317 return simple_cst_equal (TREE_OPERAND (t1, 0), t2);
7318 }
and thus finds them to be equal; the iteration in type_list_equal
continues, and runs out of parameters with t1 == t2 == NULL, and thus
returns true, and thus the two function types hash to the same slot,
and the two function types get treated as being the same.
It's not clear to me yet what the best solution to this is:
- should simple_cst_equal regard different source locations as being
different?
- should function-type hashing use a custom version of type_list_equal
when comparing params, and make different source locations of default
args be different?
- something else?
Dave
next prev parent reply other threads:[~2018-12-17 23:30 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-11-05 19:44 [PATCH 1/2] " David Malcolm
2018-11-05 19:44 ` [PATCH 2/2] C++: improvements to binary operator diagnostics (PR c++/87504) David Malcolm
2018-11-19 16:51 ` [PING] Re: [PATCH 1/2] C++: more location wrapper nodes (PR c++/43064, PR c++/43486) David Malcolm
2018-12-03 22:10 ` Jeff Law
2018-12-04 15:20 ` David Malcolm
2018-12-04 21:48 ` [PATCH 1/2] v2: " David Malcolm
2018-12-04 21:48 ` [PATCH 2/2] v2: C++: improvements to binary operator diagnostics (PR c++/87504) David Malcolm
2018-12-11 19:52 ` PING " David Malcolm
2018-12-12 20:43 ` Jason Merrill
2018-12-19 23:28 ` Aaron Sawdey
2018-12-20 2:13 ` [PATCH] -Wtautological-compare: fix comparison of macro expansions David Malcolm
2018-12-20 14:29 ` David Malcolm
2018-12-20 23:35 ` Aaron Sawdey
2018-12-04 23:31 ` [PING] Re: [PATCH 1/2] C++: more location wrapper nodes (PR c++/43064, PR c++/43486) Jason Merrill
2018-12-07 19:25 ` [PATCH 1/2] v3: " David Malcolm
2018-12-12 20:37 ` Jason Merrill
2018-12-13 19:24 ` [PATCH] v4: " David Malcolm
2018-12-13 20:38 ` Jason Merrill
2018-12-14 23:29 ` [PATCH] v5: " David Malcolm
2018-12-17 19:33 ` Jason Merrill
2018-12-17 23:30 ` David Malcolm [this message]
2018-12-18 20:23 ` Jason Merrill
2018-12-18 20:34 ` [PATCH] v6: " David Malcolm
2018-12-18 20:40 ` Jason Merrill
2018-12-19 15:35 ` David Malcolm
2018-12-19 19:01 ` [PATCH 1/2] " Thomas Schwinge
2018-12-20 2:29 ` David Malcolm
2020-03-26 5:02 ` [PATCH, OpenACC] Bug fix for processing OpenACC data clauses in C++ Sandra Loosemore
[not found] ` <4a68ec90-456a-cf49-036e-471ba275706c@codesourcery.com>
2020-03-26 14:27 ` C++ 'NON_LVALUE_EXPR' in OMP array section handling (was: [PATCH, OpenACC] Bug fix for processing OpenACC data clauses in C++) Thomas Schwinge
2020-03-26 15:09 ` C++ 'NON_LVALUE_EXPR' in OMP array section handling Sandra Loosemore
2020-03-26 20:53 ` Thomas Schwinge
2020-05-25 10:56 ` [WIP] Fold 'NON_LVALUE_EXPR' some more (was: C++ 'NON_LVALUE_EXPR' in OMP array section handling) Thomas Schwinge
2020-11-26 9:36 ` Don't create location wrapper nodes within OpenACC clauses (was: [WIP] Fold 'NON_LVALUE_EXPR' some more (was: C++ 'NON_LVALUE_EXPR' in OMP array section handling)) Thomas Schwinge
2020-11-26 10:02 ` Jakub Jelinek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1545089414.4619.239.camel@redhat.com \
--to=dmalcolm@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=jason@redhat.com \
--cc=law@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).