Re: [C++ PATCH] [PR13927] Dump alias_decl and fix duplicated error message (regression 3.4/mainline)

["Giovanni Bajo" <giovannibajo at libero dot it>]

Gabriel Dos Reis wrote:

> The way I have always understood supplement_binding is that its sole
> purpose is to implement the struct stat hack.

Yes.

> Any invalid redeclaration is handled elsewhere.  See its use throughout
> name-lookup.c for example, in particular
set_identifier_type_value_with_scope.

But the point is that invalid redeclarations should be handled *before* or
*after* supplement_binding. That is, I don't get which one would you prefer:

1) If I call supplement_binding, it means that I'm already sure that we're in a
situation where a struct hack is needed. supplement_binding will just implement
it (and abort if not possible, because it means elsewhere a check is missing).
The signature should also be changed to return nothing because it's
meaningless.

2) If I call supplement_binding, it means that I want to check if it's possible
to implement the struct hack given the current declaration or the previous one.
supplement_binding should check if it's possible, and return an error if it
isn't or implement the hack and return ok.

Which one do you think fit best?

> In C++, as opposed to C, there is only one name space for identifiers.
> Name hidding has been worked to accomodate C's struct stat hack.  A
> redeclaration is a any declaration that names a previously declared
> name.  That is the reason why you those many checks in
> duplicate_decls. In fact, that function should be splitted into two
> parts:
>   (1) one that checks for invalid redeclarations;
>   (2) one that merges attributes when a redeclaration is valid.

Thanks for the clarification. It would help if there was something like a graph
showing when all the main functions should be called and what they should do.
If you could invest a little time in doing this, I'd be *very* willing to help
cleaning up all this part.

>> You say supplement_binding is not [the correct place for errors], fine.
>> Do you want it inside duplicate_decls?
>
> Sort of.  What supplement_binding should be doing -- and that is what
> the 2/3 part of it do -- is to check for the struct stat hack
> instance and handles it locally, hands off the remaining case to
> duplicate_decls.

Another problem is that duplicate_decls is called *BEFORE* supplement_binding.
See push_{local|class|global}_binding.

> No.  First, a redeclaration is any declaration that names a previously
> declared name.  If that redeclaration is invalid then duplicate_decls
> responsaility issues a diagnostic and return error_mark_node.

OK. Then we should really walk through duplicate_decls and clean it up, at
least with respect to return value. Do you consider all the places where it
returns NULL_TREE invalid, then?

Meanwhile, a non-intrusive fix for 3.4 for the PR is just making
duplicate_decls return error_mark_node in our case, and handle the fallouts.
I'm into it.

Giovanni Bajo