ParamDecls missing from function DeclContext

I've just verified that:

1) anonymous parameters are not added to its function definition DeclContext

2) parameters are not added to its function declaration DeclContext

Are both 1 and 2 deliberate choices?

In other cases the fact that a declaration is named or anonymous is
irrelevant wrt its presence on its declaration context (and sincerely
this is what I'd expected).

In general if a declaration D has as its declaration context DC, it is a
legitimate expectation to have D listed in DC?

No, I don't think they are deliberate choices.

  - Doug

Ok, we'll provide a fix for this.

Another much related issue is shown by the following:

void sort(int (*compare)(int x, int y), int* x);

Which should be the correct context of ParmVarDecl's inside FunctionType
(i.e. int x and int y)?

I've just verified that:

1) anonymous parameters are not added to its function definition DeclContext

2) parameters are not added to its function declaration DeclContext

Are both 1 and 2 deliberate choices?

No, I don't think they are deliberate choices.

Ok, we'll provide a fix for this.

Thanks!

Another much related issue is shown by the following:

void sort(int (*compare)(int x, int y), int* x);

Which should be the correct context of ParmVarDecl's inside FunctionType
(i.e. int x and int y)?

The DeclContext notion breaks down a little bit here, because we don't create a context for FunctionTypes (and shouldn't). I'd rather that the inner ParmVarDecls be in the 'sort's FunctionDecl.

  - Doug

Also if I don't have any problem with this I'd like to understand why
you think that FunctionTypeLoc should not have a pointer to a
DeclContext where the args are registered (that for FunctionDecl
function type is the very same FunctionDecl).

Apart this issue we have added to our AST visitors an experimental
assertion to verify that if the DeclContext for declaration D is DC,
then the DeclContext DC indeed contains declaration D.

We get tons of assertion failures (e.g. for FunctionDecl or
CXXRecordDecl inside a template declaration).

We'd like to understand if this is unexpected (and in general we should
always have that if D points to DC then DC should contains D) or if we
are equivocating the overall design of DeclContext's.

Another much related issue is shown by the following:

void sort(int (*compare)(int x, int y), int* x);

Which should be the correct context of ParmVarDecl's inside
FunctionType (i.e. int x and int y)?

The DeclContext notion breaks down a little bit here, because we
don't create a context for FunctionTypes (and shouldn't). I'd rather
that the inner ParmVarDecls be in the 'sort's FunctionDecl.

Also if I don't have any problem with this I'd like to understand why
you think that FunctionTypeLoc should not have a pointer to a
DeclContext where the args are registered (that for FunctionDecl
function type is the very same FunctionDecl).

I'd rather not spend any storage on a DeclContext* in the FunctionTypeLoc.

Apart this issue we have added to our AST visitors an experimental
assertion to verify that if the DeclContext for declaration D is DC,
then the DeclContext DC indeed contains declaration D.

We get tons of assertion failures (e.g. for FunctionDecl or
CXXRecordDecl inside a template declaration).

We'd like to understand if this is unexpected (and in general we should
always have that if D points to DC then DC should contains D) or if we
are equivocating the overall design of DeclContext's.

For non-function, non-block DeclContext, this is an important invariant.

For function and block contexts, it doesn't matter: all of the declarations will be reachable via other means, because they are part of the declaration or one of the statements in the definition. If we're to do any work in the area of DeclContext for functions and blocks, I would much rather make them simpler---by eliminating both the lookup tables and the list of lexical declarations---and save the storage, rather than making them meet an invariant that doesn't seem to have any benefits.

  - Doug

The lookup tables for functions were removed by the work which Nick and I did on DeclContext name lookup in March, except for local function declarations, which don’t seem to use the Scope mechanism for name lookup. We didn’t dig deeply into that issue at the time, but it would need to be fixed before we could abandon the lexical decl chains for functions entirely.

That said, the invariant that the decls list on a DeclContext is exactly the list of Decls which have that DC as their lexical context would be useful. In particular, it could be useful for some clients to guarantee that traversing Decls by recursively iterating all lexical decls in all DCs would, in fact, reach all Decls.

Another much related issue is shown by the following:

void sort(int (compare)(int x, int y), int x);

Which should be the correct context of ParmVarDecl’s inside
FunctionType (i.e. int x and int y)?

The DeclContext notion breaks down a little bit here, because we
don’t create a context for FunctionTypes (and shouldn’t). I’d rather
that the inner ParmVarDecls be in the 'sort’s FunctionDecl.

Also if I don’t have any problem with this I’d like to understand why
you think that FunctionTypeLoc should not have a pointer to a
DeclContext where the args are registered (that for FunctionDecl
function type is the very same FunctionDecl).

I’d rather not spend any storage on a DeclContext* in the FunctionTypeLoc.

Apart this issue we have added to our AST visitors an experimental
assertion to verify that if the DeclContext for declaration D is DC,
then the DeclContext DC indeed contains declaration D.

We get tons of assertion failures (e.g. for FunctionDecl or
CXXRecordDecl inside a template declaration).

We’d like to understand if this is unexpected (and in general we should
always have that if D points to DC then DC should contains D) or if we
are equivocating the overall design of DeclContext’s.

For non-function, non-block DeclContext, this is an important invariant.

For function and block contexts, it doesn’t matter: all of the declarations will be reachable via other means, because they are part of the declaration or one of the statements in the definition. If we’re to do any work in the area of DeclContext for functions and blocks, I would much rather make them simpler—by eliminating both the lookup tables and the list of lexical declarations—and save the storage, rather than making them meet an invariant that doesn’t seem to have any benefits.

The lookup tables for functions were removed by the work which Nick and I did on DeclContext name lookup in March, except for local function declarations, which don’t seem to use the Scope mechanism for name lookup. We didn’t dig deeply into that issue at the time, but it would need to be fixed before we could abandon the lexical decl chains for functions entirely.

Local function declarations definitely need fixing. IIRC, they still have the wrong semantic declaration context (as do local extern variables). And if we fix them, we can refactor just a little to kill off that LookupPtr in FunctionDecl’s DeclContext (e.g., by splitting DeclContext into DeclContext and SearchableDeclContext).

That said, the invariant that the decls list on a DeclContext is exactly the list of Decls which have that DC as their lexical context would be useful. In particular, it could be useful for some clients to guarantee that traversing Decls by recursively iterating all lexical decls in all DCs would, in fact, reach all Decls.

Okay, I agree that this is a useful general invariant.

  • Doug

    >>
    >>
    >>> Another much related issue is shown by the following:
    >>>
    >>> void sort(int (*compare)(int x, int y), int* x);
    >>>
    >>> Which should be the correct context of ParmVarDecl's inside
    >>> FunctionType (i.e. int x and int y)?
    >>
    >>
    >> The DeclContext notion breaks down a little bit here, because we
    >> don't create a context for FunctionTypes (and shouldn't). I'd
    rather
    >> that the inner ParmVarDecls be in the 'sort's FunctionDecl.
    >
    > Also if I don't have any problem with this I'd like to
    understand why
    > you think that FunctionTypeLoc should not have a pointer to a
    > DeclContext where the args are registered (that for FunctionDecl
    > function type is the very same FunctionDecl).

    I'd rather not spend any storage on a DeclContext* in the
    FunctionTypeLoc.

    > Apart this issue we have added to our AST visitors an experimental
    > assertion to verify that if the DeclContext for declaration D is DC,
    > then the DeclContext DC indeed contains declaration D.
    >
    > We get tons of assertion failures (e.g. for FunctionDecl or
    > CXXRecordDecl inside a template declaration).
    >
    > We'd like to understand if this is unexpected (and in general we
    should
    > always have that if D points to DC then DC should contains D) or
    if we
    > are equivocating the overall design of DeclContext's.

    For non-function, non-block DeclContext, this is an important
    invariant.

    For function and block contexts, it doesn't matter: all of the
    declarations will be reachable via other means, because they are
    part of the declaration or one of the statements in the
    definition. If we're to do any work in the area of DeclContext for
    functions and blocks, I would much rather make them simpler---by
    eliminating both the lookup tables and the list of lexical
    declarations---and save the storage, rather than making them meet
    an invariant that doesn't seem to have any benefits.

The lookup tables for functions were removed by the work which Nick
and I did on DeclContext name lookup in March, except for local
function declarations, which don't seem to use the Scope mechanism for
name lookup. We didn't dig deeply into that issue at the time, but it
would need to be fixed before we could abandon the lexical decl chains
for functions entirely.

Local function declarations definitely need fixing. IIRC, they still
have the wrong semantic declaration context (as do local extern
variables). And if we fix them, we can refactor just a little to kill
off that LookupPtr in FunctionDecl's DeclContext (e.g., by splitting
DeclContext into DeclContext and SearchableDeclContext).

That said, the invariant that the decls list on a DeclContext is
exactly the list of Decls which have that DC as their lexical context
would be useful. In particular, it could be useful for some clients to
guarantee that traversing Decls by recursively iterating all lexical
decls in all DCs would, in fact, reach all Decls.

Okay, I agree that this is a useful general invariant.

Everything is very nice in this way and we'd love to do in this way, but
this contrasts with

Abramo wrote:
Also if I don't have any problem with this I'd like to understand why
you think that FunctionTypeLoc should not have a pointer to a
DeclContext where the args are registered (that for FunctionDecl
function type is the very same FunctionDecl).

Doug answer:
I'd rather not spend any storage on a DeclContext* in the FunctionTypeLoc.

Suppose we have:
typedef int f(int x);

We should assign to int x a lexical DeclContext, suppose we choose to
assign it to the TranslationUnit... then to satisfy the invariant above
we should insert the ParmVarDecl in the TranslationUnit.

I really doubt this is sane choice... what happens to all visitors of
DeclContext when they encounter these ParmVarDecl (that I'd say are
definitely "misplaced"...)

The C(++) languages don't actually place such declarations in any particular context, so it's not clear where they belong in the AST. Introducing some kind of abstract function declaration that is the DeclContext might conceivably work, but that's a rather heavyweight solution just to maintain an invariant. Using FunctionTypeLoc doesn't really work, because DeclContexts are supposed to be Decls.

Personally, I'm fine with having parameters in the lexical DeclContext of the translation unit for this case, but we can investigate other designs.

  - Doug

Another much related issue is shown by the following:

void sort(int (*compare)(int x, int y), int* x);

Which should be the correct context of ParmVarDecl's inside
FunctionType (i.e. int x and int y)?

The DeclContext notion breaks down a little bit here, because we
don't create a context for FunctionTypes (and shouldn't). I'd

   rather

that the inner ParmVarDecls be in the 'sort's FunctionDecl.

Also if I don't have any problem with this I'd like to

   understand why

you think that FunctionTypeLoc should not have a pointer to a
DeclContext where the args are registered (that for FunctionDecl
function type is the very same FunctionDecl).

   I'd rather not spend any storage on a DeclContext* in the
   FunctionTypeLoc.

Apart this issue we have added to our AST visitors an experimental
assertion to verify that if the DeclContext for declaration D is DC,
then the DeclContext DC indeed contains declaration D.

We get tons of assertion failures (e.g. for FunctionDecl or
CXXRecordDecl inside a template declaration).

We'd like to understand if this is unexpected (and in general we

   should

always have that if D points to DC then DC should contains D) or

   if we

are equivocating the overall design of DeclContext's.

   For non-function, non-block DeclContext, this is an important
   invariant.

   For function and block contexts, it doesn't matter: all of the
   declarations will be reachable via other means, because they are
   part of the declaration or one of the statements in the
   definition. If we're to do any work in the area of DeclContext for
   functions and blocks, I would much rather make them simpler---by
   eliminating both the lookup tables and the list of lexical
   declarations---and save the storage, rather than making them meet
   an invariant that doesn't seem to have any benefits.

The lookup tables for functions were removed by the work which Nick
and I did on DeclContext name lookup in March, except for local
function declarations, which don't seem to use the Scope mechanism for
name lookup. We didn't dig deeply into that issue at the time, but it
would need to be fixed before we could abandon the lexical decl chains
for functions entirely.

Local function declarations definitely need fixing. IIRC, they still
have the wrong semantic declaration context (as do local extern
variables). And if we fix them, we can refactor just a little to kill
off that LookupPtr in FunctionDecl's DeclContext (e.g., by splitting
DeclContext into DeclContext and SearchableDeclContext).

That said, the invariant that the decls list on a DeclContext is
exactly the list of Decls which have that DC as their lexical context
would be useful. In particular, it could be useful for some clients to
guarantee that traversing Decls by recursively iterating all lexical
decls in all DCs would, in fact, reach all Decls.

Okay, I agree that this is a useful general invariant.

Everything is very nice in this way and we'd love to do in this way, but
this contrasts with

Abramo wrote:
Also if I don't have any problem with this I'd like to understand why
you think that FunctionTypeLoc should not have a pointer to a
DeclContext where the args are registered (that for FunctionDecl
function type is the very same FunctionDecl).

Doug answer:
I'd rather not spend any storage on a DeclContext* in the FunctionTypeLoc.

Suppose we have:
typedef int f(int x);

We should assign to int x a lexical DeclContext, suppose we choose to
assign it to the TranslationUnit... then to satisfy the invariant above
we should insert the ParmVarDecl in the TranslationUnit.

I really doubt this is sane choice... what happens to all visitors of
DeclContext when they encounter these ParmVarDecl (that I'd say are
definitely "misplaced"...)

The C(++) languages don't actually place such declarations in any
particular context, so it's not clear where they belong in the AST.
Introducing some kind of abstract function declaration that is the
DeclContext might conceivably work, but that's a rather heavyweight
solution just to maintain an invariant. Using FunctionTypeLoc doesn't
really work, because DeclContexts are supposed to be Decls.

Just to be clear, I was not thinking to transform FunctionTypeLoc in a
DeclContext, but to put a DeclContext* in FunctionTypeLoc data.

This pointer points to FunctionDecl itself when the FunctionTypeLoc is
the type of a FunctionDecl, otherwise it points to a specific class
derived from DeclContext (FunctionTypeDeclContext?) that contains only
the ParmVarDecl's.

Personally, I'm fine with having parameters in the lexical
DeclContext of the translation unit for this case, but we can investigate other designs.

How you would skip them?

What about template instantiation of these? Everything would be added to
TranslationUnit?

Perhaps I'm the only to feel this way, but to add Decls to contexts
where they don't belong sincerely appears like a little mess to me...

That said, the invariant that the decls list on a DeclContext is
exactly the list of Decls which have that DC as their lexical context
would be useful. In particular, it could be useful for some clients to
guarantee that traversing Decls by recursively iterating all lexical
decls in all DCs would, in fact, reach all Decls.

Okay, I agree that this is a useful general invariant.

Everything is very nice in this way and we'd love to do in this way, but
this contrasts with

Abramo wrote:
Also if I don't have any problem with this I'd like to understand why
you think that FunctionTypeLoc should not have a pointer to a
DeclContext where the args are registered (that for FunctionDecl
function type is the very same FunctionDecl).

Doug answer:
I'd rather not spend any storage on a DeclContext* in the FunctionTypeLoc.

Suppose we have:
typedef int f(int x);

We should assign to int x a lexical DeclContext, suppose we choose to
assign it to the TranslationUnit... then to satisfy the invariant above
we should insert the ParmVarDecl in the TranslationUnit.

I really doubt this is sane choice... what happens to all visitors of
DeclContext when they encounter these ParmVarDecl (that I'd say are
definitely "misplaced"...)

The C(++) languages don't actually place such declarations in any
particular context, so it's not clear where they belong in the AST.
Introducing some kind of abstract function declaration that is the
DeclContext might conceivably work, but that's a rather heavyweight
solution just to maintain an invariant. Using FunctionTypeLoc doesn't
really work, because DeclContexts are supposed to be Decls.

Just to be clear, I was not thinking to transform FunctionTypeLoc in a
DeclContext, but to put a DeclContext* in FunctionTypeLoc data.

This pointer points to FunctionDecl itself when the FunctionTypeLoc is
the type of a FunctionDecl, otherwise it points to a specific class
derived from DeclContext (FunctionTypeDeclContext?) that contains only
the ParmVarDecl's.

In the FunctionDecl case, we don't actually need this pointer because it is available in the ParmVarDecls. Can we optimize it away?

Personally, I'm fine with having parameters in the lexical
DeclContext of the translation unit for this case, but we can investigate other designs.

How you would skip them?

isa<ParmVarDecl>?

What about template instantiation of these? Everything would be added to
TranslationUnit?

I guess so.

Perhaps I'm the only to feel this way, but to add Decls to contexts
where they don't belong sincerely appears like a little mess to me...

I agree that it feels a little messy, but we're discussing increasing Clang's memory footprint to meet an invariant that we've never needed before. Make it cheap or make it important and it becomes obvious that it's worth doing.

  - Doug

It’s not completely clear to me what invariant a FunctionTypeDeclContext would help with. It seems to be attempting to make the DeclContext structure reflect the unqualified name lookup rules? We are far from that being an invariant (for instance, at block scope, or when a tag type is declared within a declarator). For instance, if we visit this TU:

typedef void f(struct S);

… we visit a ‘struct S;’ declaration within the TU, before we visit the ‘typedef’ declaration.

I think the intent is that the FunctionTypeDeclContext be the context used for parameters in a function type that is not directly the type of a function or function template declaration. One could then iterate over the lexical declarations in the FunctionTypeDeclContext to get the ParmVarDecls in that function type.

  • Doug

Does even this give us a reasonable AST? Okay, great, the FunctionTypeDeclContext is the semantic context of these declarations. And its parent context is… the translation unit?

I think the truth is that parameters of abstract function types are a weird exception, because they’re actually only declarations grammatically; they don’t really declare anything. I would rather give them no decl context than give them some pretend one. This is also a much easier invariant to maintain: a ParmVarDecl has a DC if and only if it declares a parameter of an actual function declaration.

John.

The translation unit is a safer form of “no decl context”, given that we assume throughout the frontend that Decl::getDeclContext() returns non-NULL. I guess we could invent some other sentinel DeclContext for this purpose.

  • Doug