C++ PATCH [3.3.3] PR 13544

[Gabriel Dos Reis]

Mark Mitchell <mark@codesourcery.com> writes:

| On Sun, 2004-01-11 at 12:12, Gabriel Dos Reis wrote:
| > Jason Merrill <jason@redhat.com> writes:
| > 
| > | On 11 Jan 2004 01:39:29 +0100, Gabriel Dos Reis <gdr@integrable-solutions.net> wrote:
| > | 
| > | >   (1) DECL_NAMESPACE_SCOPE_P does not actually test whether a decl has
| > | >       a namespace scope -- because if teh DECL_CONTEXT is absent then
| > | >       it defaults to the global_namespace;
| > | 
| > | Yes, it does.  It uses CP_DECL_CONTEXT, which returns global_namespace as
| > | appropriate.
| > 
| > The point in the issue was precisely that that default was not appropriate.
| > 
| > It does not really test whether a decl has namespace scope. 
| 
| Perhaps you are making the technical point that the global scope is not
| a namespace?

No.  What I'm saying is that if DECL_NAMESPACE_SCOPE_P is applied to a
template parameter, for example, it will yield true -- instead of
false (the right answer), since template parameters don't have a
"natural" scope.  In the comment (in pushdecl) above its use, we have:

      /* In case this decl was explicitly namespace-qualified, look it
         up in its namespace context.  */

and the code uses DECL_NAMESPACE_SCOPE_P to test for that  explicit
qualification (which cannot be true, in this case).
Furthermore, if pushdecl() should be setting DECL_CONTEXT (as you also
seem to agree on) then that testing is not right because in case
DECL_CONTEXT is null (not yet set), DECL_NAMESPACE_SCOPE_P will yield
true. 

What I'm trying to say is that the defaulting to global_namespace is
not appropriate for some cases (and I don't even thing it is right at
all), and that can lead to subtle bugs. 

| The compiler treats it as if it were throughout, and that works well. 

Except when it does not, like in this case :-) 

| DECL_NAMESPACE_SCOPE_P is perhaps inaccurately named, from that
| technical point of view, but that's a very minor technicality.
| 
| I'm not sure about your patch.  I think pushdecl *should* set
| DECL_CONTEXT in the abstract -- or else we should always depend on
| callers to set it.  It's not very good to do one thing in some places
| and the other thing in others.

FWIW, I fully agree with you that pushdecl should be responsible to
set DECL_CONTEXT -- just like finish_member_declaration does, see my
comment in build_enumerator.  That is what I tried first to do.
However the resulting patch was too invasive and I was not even sure
it would not cause another regression; therefore I falled back to the
more conservative approach I produced.  I didn't really like it but,
The Right approach may cause more radical changes which might not be
appropriate for this late time. 

-- Gaby