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

[Gabriel Dos Reis <gdr at integrable-solutions dot net>]

"Giovanni Bajo" <giovannibajo@libero.it> writes:

[...]

| > In particular, supplement_binding as the comment says should not be
| > messing with diagnostics.  If you want to violate that then you need
| > to rework the entire stuff (I mean duplicate_decls).
| > Supplement_binding assumes that when it is called it is valid to
| > supplement the binding, and that the checking logic is done somewhere
| > else.
| 
| You told me this. My observations, rephrased:

I think you should have included the example contained in the original
mail.   I'm putting it here again

      struct S { };
      namespace S { }

| 1) What do you mean when you say "supplement_binding assumes that when it is
| called it is valid to supplement the binding"? Do you mean that, when we call
| it, the supplement binding should be always succesfull? What do you mean with
| "successfull"? supplement_binding has a return value: "Returns nonzero if the
| new binding was successful". Does your sentence mean that this should be
| removed (in the longer run)?

The way I have always understood supplement_binding is that its sole
purpose is to implement the struct stat hack.  Any invalid
redeclaration is handled elsewhere.  See its use throughout
name-lookup.c for example, in particular
set_identifier_type_value_with_scope.

| 2) The comment at the top of duplicate_decls says: "If NEWDECL is a
| redeclaration of OLDDECL, merge the declarations. If the redeclaration is
| invalid, a diagnostic is issued, and the error_mark_node is returned.
| Otherwise, OLDDECL is returned. If NEWDECL is not a redeclaration of OLDDECL,
| NULL_TREE is returned." When I read this, I understand that if newdecl is *not*
| a redecleration of olddecl, a diagnostic should not be emitted. In fact,
| duplicate_decls is very inconsistent about when to emit a diagnostic.

If you want to reword that stuff, I'm all for it.   But first of all,
we need to agree on what is called redeclaration.  I do not believe
it should be the business of suppement_binding to check for invalid
redeclarations.  What I mean what invalid redeclaration is the kind of
example I gave above.  Probably one of the source of confusion is that
duplicate_decls is a vestige from the ear when the C++ front-end
started as a copy-and-paste of  the C front-end.

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.

| 3) Which is the correct place for this diagnostic "redeclared as
| different kind of symbol"? 

The right place is duplicate_decls.  But as said above, it would
probably need to be reworked (that is needed any way to make its logic
consistent. 

| You say supplement_binding is not, 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.

| But there is a problem, because if duplicate_decls returns
| NULL_TREE on it (and it has to, since they are different symbols), then
| push_[local/class/namespace]_binding *will* try to call supplement_binding on

You will not return a NULL tree.  You'll return an error_mark_node
since you're going to issue a diagnostic. 

| it. After all, it's what it's for. Would you prefer a common error
| function to 
| emit a diagnostic for an invalid redeclaration whenever supplement_binding
| returns false? This would make it something like: if (!supplement_binding())
| error(), which is not totally different from emitting the diagnostic within
| supplement_binding itself, but anyway.

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.  

| 4) You say that "if you want to emit a diagnostic in supplement_binding, you
| need to rework the entire duplicate_decls". Would you please elaborate on this?
| It's not clear to me why duplicate_decls would have to be heavily changed. An
| example would help.

I gave you an example that illustrates the problem when you sent me
the private email. You choosed to snip when replying in public.
The reason you need to reword duplicate_decls is that the logic you
would be putting in supplement_binding would be for most of them
already in duplicate_decls and if you don't rework duplicate_decls
then you'll end up with duplicate  diagnostics or inconsitent one.
(One of the reasons you get the duplicate diagnostic is precisely that
both are trying to do the same thing, with one violating what its
description says.)