C++ PATCH [3.3.3] PR 13544
Gabriel Dos Reis
gdr@integrable-solutions.net
Mon Jan 12 05:45:00 GMT 2004
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
More information about the Gcc-patches
mailing list