On Tue, 2018-12-18 at 15:40 -0500, Jason Merrill wrote:
> On 12/18/18 4:22 PM, David Malcolm wrote:
> > On Mon, 2018-12-17 at 18:30 -0500, David Malcolm wrote:
> > > 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
> >
> > I tried both of the above approaches, and both work.
> >
> > Here's v6 of the patch:
> >
> > I removed the strip of wrappers in
> > cp_parser_late_parsing_default_args
> > from earlier versions of the patch, in favor of fixing
> > simple_cst_equal
> > so that it treats location wrappers with unequal source locations
> > as
> > being unequal. This ensures that function-types with default
> > arguments
> > don't get merged when the default argument constants have different
> > spelling locations. [I have an alternative patch which instead
> > introduces a different comparator for FUNCTION_TYPE's
> > TYPE_ARG_TYPES
> > within type_cache_hasher::equal, almost identical to
> > type_list_equal,
> > but adding the requirement that location wrappers around default
> > arguments have equal source location for the params to be
> > considered
> > equal; both patches pass bootstrap®ression testing]
> >
> > Doing so leads to the reported location for the bad default
> > argument
> > within a template in g++.dg/template/defarg6.C moving to the
> > argument
> > location. Previously, the callsite of the instantiation was
> > identified
> > due to the use of input_location in convert_like_real here:
> >
> > 6816 location_t loc = cp_expr_loc_or_loc (expr,
> > input_location);
> >
> > With a location wrapper, it uses the spelling location of the
> > default argument, but doesn't identify the location of the callsite
> > that's instantiating the template.
> >
> > So I moved the note in tsubst_default_argument about which callsite
> > led to a diagnostic to after the check_default_argument call, so
> > that
> > diagnostics within that receive notes, too.
> >
> > As before, this was successfully bootstrapped & regrtested on
> > x86_64-pc-linux-gnu, in conjunction with the followup patch.
> >
> > OK for trunk?
>
> Ah, I hadn't seen this before my last email. Let's go with this
> version.
Thanks.
I've now committed the v6 patch, and the follow-ups that were already
approved (having rebased and retested them):
r267272:
"C++: more location wrapper nodes (PR c++/43064, PR c++/43486)"
r267273:
"C++: improvements to binary operator diagnostics (PR c++/87504)"
r267276:
"C++: better locations for bogus initializations (PR c++/88375)"
Dave
