[Bug c++/42356] confused compiler

[Bug c++/42356] confused compiler

[paolo.carlini at oracle dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42356

Paolo Carlini <paolo.carlini at oracle dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|gcc-bugs at gcc dot gnu.org |

--- Comment #6 from Paolo Carlini <paolo.carlini at oracle dot com> 2011-09-28 21:21:40 UTC ---
For the record, ICC also thinks it's ambiguous.

[Bug c++/42356] confused compiler

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42356

Manuel López-Ibáñez <manu at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2011-10-22
                 CC|                            |manu at gcc dot gnu.org
     Ever Confirmed|0                           |1

--- Comment #7 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2011-10-22 12:27:11 UTC ---
Clang prints:

/tmp/webcompile/_26276_0.cc:17:19: error: no matching member function for call
to 'newNode'
    bar*    b = f.newNode<bar>();
                ~~^~~~~~~~~~~~
/tmp/webcompile/_26276_0.cc:6:13: note: candidate function template not viable:
requires 1 argument, but 0 were provided
    T*      newNode(U u) { return newNode()->init(u); }
            ^
/tmp/webcompile/_26276_0.cc:6:13: note: candidate function template not viable:
requires 1 argument, but 0 were provided
/tmp/webcompile/_26276_0.cc:8:13: note: candidate function template not viable:
requires 2 arguments, but 0 were provided
    T*      newNode(U u, V v) { return newNode()->init(u, v); }
            ^
/tmp/webcompile/_26276_0.cc:8:13: note: candidate function template not viable:
requires 2 arguments, but 0 were provided

which is nicer except for the duplicated messages.

The first thing that should go away is the "expected primary-expression" stuff.

[Bug c++/42356] improve list of candidates and error recovery for ambiguous call

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42356

Manuel López-Ibáñez <manu at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|confused compiler           |improve list of candidates
                   |                            |and error recovery for
                   |                            |ambiguous call
           Severity|normal                      |enhancement

[Bug c++/42356] improve list of candidates and error recovery for ambiguous call

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42356

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-10-22 13:00:32 UTC ---
(In reply to comment #2)
> Well, I see several issues with the diagnostics. 
> 
> 1) The call is not ambiguous, because if it were (only) ambiguous then removing
> the base "baz" would disambiguate. Instead the case without "baz" gets you:
>    s3:~/ootbc/memspec$ g++ foo.cc
>    foo.cc: In function ‘int main()’:
>    foo.cc:27: error: no matching function for call to ‘foo::newNode()’
> That is, a call cannot be both ambiguous and have no matching function.

The call is ambiguous, because the name is found in two different base classes,
see the rules in [class.member.lookup].  Removing one of the bases means the
names are only found from one scope, and so overload resolution can proceed,
which fails because there are no viable functions.

Name lookup happens first, then overload resolution.
In your case the result of name lookup is ambiguous, like this:

struct A {
  void f(int);
};
struct B {
  void f(double);
};
struct C : A, B { };

int main() {
  C c;
  c.f(0);
}

even though A::f(int) is a better match, the call is ambiguous.

Clang's diagnostic refers to viable functions, which is wrong, because finding
viable functions happens as part of overload resolution, which shouldn't even
happen here because name lookup is ambiguous.

> 2) The reported list of overloads include those which have the wrong number of
> arguments.

That's by design. Maybe that's the function you meant to call.  If you call a
function with the wrong number of args you want the compiler to tell you name
lookup found something, but overload resolution failed because the number of
arguments didn't match.

> 3) The diagnostic contains "freeList::newNode(U, V) [with U = U, V = V, T =
> bar]" as the call citation, which makes it look like the local U matches a
> (non-existent in this case) global U and local V matches a global V, the way
> that local T matches global bar. Perhaps it could say something like
> "freeList::newNode(U, V) [with U = ?, V = ?, T = bar]"

That might be an improvement, yes.  That's the only issue I see here.

[Bug c++/42356] improve list of candidates and error recovery for ambiguous call

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42356

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-10-22 13:07:50 UTC ---
(In reply to comment #8)
> That might be an improvement, yes.  That's the only issue I see here.

Actually, there is another issue in the list of candidates:

template<class U> T* freeList::newNode(U) [with U = U, T = baz]
T* freeList<T>::newNode() [with T = baz]

The first one should say freeList<T> not freeList.

That would help realise that the member name has been found in two classes.

With -fno-pretty-templates it's better:

f.C:17:17: error: request for member ‘newNode’ is ambiguous
f.C:8:15: error: candidates are: template<class U, class V> baz*
freeList<baz>::newNode<U, V>(U, V)
f.C:6:15: error:                 template<class U> baz*
freeList<baz>::newNode<U>(U)
f.C:4:13: error:                 baz* freeList<baz>::newNode()
f.C:8:15: error:                 template<class U, class V> bar*
freeList<bar>::newNode<U, V>(U, V)
f.C:6:15: error:                 template<class U> bar*
freeList<bar>::newNode<U>(U)
f.C:4:13: error:                 bar* freeList<bar>::newNode()

This makes it easier to see that name lookup found these functions:
  freeList<baz>::newNode(U, V)
  freeList<baz>::newNode(U)
  freeList<bar>::newNode(U,V)
  freeList<bar>::newNode(U)
so you can see they come from different scopes (it would be even clearer if
"bar" and "baz" weren't visually very similar!)

[Bug c++/42356] improve list of candidates and error recovery for ambiguous call

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42356

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-10-22 13:21:11 UTC ---
(In reply to comment #9)
> The first one should say freeList<T> not freeList.

reported as PR 50828

[Bug c++/42356] improve list of candidates and error recovery for ambiguous call

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42356

--- Comment #11 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2011-10-22 13:30:10 UTC ---
(In reply to comment #8)
> > 2) The reported list of overloads include those which have the wrong number of
> > arguments.
> 
> That's by design. Maybe that's the function you meant to call.  If you call a
> function with the wrong number of args you want the compiler to tell you name
> lookup found something, but overload resolution failed because the number of
> arguments didn't match.

I wonder why the detailed overload failure that Nathan implemented does not
trigger here. I would expect to give details of why overload failed.

g++ could also specify which ones are viable candidates, and which ones are not
even viable, and for the ones not viable, explain why.

> That might be an improvement, yes.  That's the only issue I see here.

There is also the issue of the "expected primary-expression" triggered two
times.

[Bug c++/42356] improve list of candidates and error recovery for ambiguous call

[igodard at pacbell dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42356

--- Comment #12 from Ivan Godard <igodard at pacbell dot net> 2011-10-22 15:52:54 UTC ---
Manual said:
g++ could also specify which ones are viable candidates, and which ones are not
even viable, and for the ones not viable, explain why.

[Bug c++/42356] improve list of candidates and error recovery for ambiguous call

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42356

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-10-22 19:43:08 UTC ---
(In reply to comment #11)
> I wonder why the detailed overload failure that Nathan implemented does not
> trigger here. I would expect to give details of why overload failed.

Name lookup fails due to the ambiguity, so overload resolution never happens. 
The detailed overload resolution diagnostics won't be printed if overload
resolution isn't done.

> g++ could also specify which ones are viable candidates, and which ones are not
> even viable, and for the ones not viable, explain why.

There are no viable candidates, because overload resolution is not performed.

Why bother performing overload resolution if the result of name lookup is
ambiguous?  How does it help you resolve the ambiguity?

The ambiguity can be resolved by qualifying the name or with a using
declaration, but overload resolution is irrelevant for either of them: you
qualify a *name* and a using declaration names a *name*, not an overload

[Bug c++/42356] improve list of candidates and error recovery for ambiguous call

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42356

--- Comment #14 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2011-10-23 00:46:15 UTC ---
(In reply to comment #13)
> (In reply to comment #11)
> > I wonder why the detailed overload failure that Nathan implemented does not
> > trigger here. I would expect to give details of why overload failed.
> 
> Name lookup fails due to the ambiguity, so overload resolution never happens. 
> The detailed overload resolution diagnostics won't be printed if overload
> resolution isn't done.

So you are saying that the candidates printed are simply all the matching names
that name lookup is able to find?

If so, I understand that the way g++ implements overload resolution makes not
trivial to give diagnostics, but still as Ivan agrees, it would be nice to be
able to sort viable candidates first, then not viable with details about why
they are not. This would require factoring out code to test overloads and
report diagnostics.

> > g++ could also specify which ones are viable candidates, and which ones are not
> > even viable, and for the ones not viable, explain why.
> 
> There are no viable candidates, because overload resolution is not performed.
> 
> Why bother performing overload resolution if the result of name lookup is
> ambiguous?  How does it help you resolve the ambiguity?

Yourself explained why above:

"Maybe that's the function you meant to call.  If you call a function with the
wrong number of args you want the compiler to tell you name lookup found
something, but overload resolution failed because the number of arguments
didn't match."

[Bug c++/42356] improve list of candidates and error recovery for ambiguous call

[manu at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42356

--- Comment #15 from Manuel López-Ibáñez <manu at gcc dot gnu.org> 2011-10-23 00:47:42 UTC ---
(In reply to comment #12)
> Manual said:
> g++ could also specify which ones are viable candidates, and which ones are not
> even viable, and for the ones not viable, explain why.

http://en.wikipedia.org/wiki/Manual

http://en.wikipedia.org/wiki/Manuel

[Bug c++/42356] improve list of candidates and error recovery for ambiguous call

[igodard at pacbell dot net]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42356

--- Comment #16 from Ivan Godard <igodard at pacbell dot net> 2011-10-23 01:28:48 UTC ---
(In reply to comment #15)
> (In reply to comment #12)
> > Manual said:
> > g++ could also specify which ones are viable candidates, and which ones are not
> > even viable, and for the ones not viable, explain why.
> 
> http://en.wikipedia.org/wiki/Manual
> 
> http://en.wikipedia.org/wiki/Manuel

Whoever said it, they were right :-)

[Bug c++/42356] improve list of candidates and error recovery for ambiguous call

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42356

--- Comment #17 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-10-23 01:54:17 UTC ---
But for this testcase I don't want to be told overload resolution passed or
failed, I want to be told it's ambiguous, because that's the error in the
testcase that prevents it compiling, and the diagnostic should help fix that
problem.

You say clang's output is nicer, I think you should look again.  Notice there
are 6 overloads:
freeList<bar>f(), freeList<bar>f(U), and freeList<bar>f(U,V),
freeList<baz>f(), freeList<baz>f(U), and freeList<baz>f(U,V),

Clang prints 4 errors (which appear to be duplicates, but actually refer to
freeList<bar> and freeList<baz> but that's not shown).  Why are the other two
overloads not shown?  Why doesn't it mention that the call is ambiguous?
It fails to state the problem, and misleadingly implies overload resolution
failed.

G++ mentions all six and correctly says they're ambiguous.  It would be better
if it said why (name lookup found "newNode" in multiple base classes) as clang
does for this code (which it gets right):

template<class T>
struct A {
template<class U>
  void f(U);
};
struct C : A<void>, A<int> { };

int main() {
  C c;
  c.f<char>();
}

[Bug c++/42356] improve list of candidates and error recovery for ambiguous call

[redi at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42356

--- Comment #18 from Jonathan Wakely <redi at gcc dot gnu.org> 2011-10-23 02:03:41 UTC ---
(In reply to comment #12)
> Manual said:
> g++ could also specify which ones are viable candidates, and which ones are not
> even viable, and for the ones not viable, explain why.

None of the functions are viable, because 'newNode' is ambiguous.


So your suggestion seems to be:

request for member is ambiguous
Overload 1: not viable, reason: ambiguous.
Overload 2: not viable, reason: ambiguous.
Overload 3: not viable, reason: ambiguous.
Overload 4: not viable, reason: ambiguous.
Overload 5: not viable, reason: ambiguous.
Overload 6: not viable, reason: ambiguous.

rather than what we have now:

request for member is ambiguous
Overload 1
Overload 2
Overload 3
Overload 4
Overload 5
Overload 6
[unhelpful nonsense about expected primary expression]

I think the second one is better (apart from the primary expression part)

Or maybe you could have:

request for member is ambiguous
Overload 1: not viable, reason: ambiguous.
Overload 2: not viable, reason: ambiguous.
Overload 3: not viable, reason: ambiguous, and wrong number of args
Overload 4: not viable, reason: ambiguous, and wrong number of args
Overload 5: not viable, reason: ambiguous, and wrong number of args
Overload 6: not viable, reason: ambiguous, and wrong number of args

I don't think that helps - changing the number of arguments wouldn't make the
testcase compile. Resolving the ambiguous lookup would.

[Bug c++/42356] improve list of candidates and error recovery for ambiguous call

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42356

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|2011-10-22 00:00:00         |2021-9-7

--- Comment #19 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This is what clang now prints:
<source>:18:19: error: member 'newNode' found in multiple base classes of
different types
    bar*    b = f.newNode<bar>();
                  ^
<source>:5:13: note: member found by ambiguous name lookup
    T*      newNode() { return 0; }
            ^
<source>:5:13: note: member found by ambiguous name lookup

Which is better than GCC's but does not print out what the multiple base
classes are.