[Patch] IDNS_NoLookup?

Clang Frontend
1

Hi,

I would like to introduce new identifier namespace, which would cause
ScopedDecl not be added to DeclContext lookup structure. Currently it
IdentifierResolver saves us somehow from building lookup structure for
TranslationUnit, however, as i mentioned few weeks earlier I store
using-directives in lookup structure. This causes it to be build for
TranslationUnitDecl too in my local copy. This fails for
ObjCImplementation (there might be more somewhere in ObjC code, not
really sure...), which has associated name but getIdentifierNamespace
is not defined for it. I could probably workaround it, but this way
feels `less hackish`. Please note that this change also saves from
crash client code, that would call TranslationUnitDecl::lookup(), when
it owns ObjCImplementationDecls.

Attached tiny diff with that change.

Piotr

IDNS_NoLookup.diff (1.05 KB)

2

Hi Piotr,

I would like to introduce new identifier namespace, which would cause
ScopedDecl not be added to DeclContext lookup structure. Currently it
IdentifierResolver saves us somehow from building lookup structure for
TranslationUnit, however, as i mentioned few weeks earlier I store
using-directives in lookup structure. This causes it to be build for
TranslationUnitDecl too in my local copy.

using-directives won't have names, right? So it doesn't really matter that they won't have any identifier namespace defined for them.

This fails for
ObjCImplementation (there might be more somewhere in ObjC code, not
really sure...), which has associated name but getIdentifierNamespace
is not defined for it. I could probably workaround it, but this way
feels `less hackish`. Please note that this change also saves from
crash client code, that would call TranslationUnitDecl::lookup(), when
it owns ObjCImplementationDecls.

Ah, so that's the issue... the ObjCImplementationDecls need to have an identifier namespace, because they have name, but they aren't supposed to be found by name lookup (rather, we see them through the ObjCInterfaceDecl).

Adding another namespace will work, but I don't know if it's the best answer. Perhaps what we really mean is that ObjCImplementationDecls really don't have names at all: they're just unnamed entities that attach to other entities, and the only reason they have a name in the source code is to tie them to the appropriate interface:

@interface Blah
@end

@implementation Blah /* ties this ObjCImplementationDecl to the ObjCInterfaceDecl above */
@end

One could certainly ask an ObjCImplementationDecl what the name of its corresponding interface is, through a separate operation, even though the ObjCImplementationDecl itself is unnamed.

I'm not quite sure which way to go with this. IDNS_NoLookup works, but I think an unnamed ObjCImplementationDecl will be better. Let's see if someone with a better understanding of Objective-C semantics can chime in to see if my understanding of ObjCImplementationDecl is reasonable.

  - Doug

3

Ah, very interesting. I think the right fix is that ObjCImplementationDecl should not be scope decls and maybe not even NamedDecls. They should never ever be "looked up".

Steve, Fariborz, what do you guys think?

-Chris

4

I think we should swap ScopedDecl and NamedDecl in the inheritance hierarchy. Things without names---like, perhaps, ObjCImplementationDecl---should still be ScopedDecls and live inside DeclContexts (so we know where they occur, lexically, in the program, and who their owners are) but they don't need any storage for names. So, we would just have

   class NamedDecl : public ScopedDecl {
     DeclarationName Name;
   public:
     // ...
   };

Right now, the only ScopedDecl that isn't a NamedDecl is OverloadedFunctionDecl, and it's probably going away in favor of Argiris' idea for an OverloadedNameExpr. As Piotr had mentioned before, using directives are also never found by name lookup, but they do have scope, so they're natural ScopedDecls but not NamedDecls.

  - Doug

5

That makes a lot of sense to me!

-Chris

6

Hi Piotr,

I would like to introduce new identifier namespace, which would cause
ScopedDecl not be added to DeclContext lookup structure. Currently it
IdentifierResolver saves us somehow from building lookup structure for
TranslationUnit, however, as i mentioned few weeks earlier I store
using-directives in lookup structure. This causes it to be build for
TranslationUnitDecl too in my local copy.

using-directives won't have names, right? So it doesn't really matter
that they won't have any identifier namespace defined for them.

This fails for
ObjCImplementation (there might be more somewhere in ObjC code, not
really sure...), which has associated name but getIdentifierNamespace
is not defined for it. I could probably workaround it, but this way
feels `less hackish`. Please note that this change also saves from
crash client code, that would call TranslationUnitDecl::lookup(), when
it owns ObjCImplementationDecls.

Ah, so that's the issue... the ObjCImplementationDecls need to have an
identifier namespace, because they have name, but they aren't
supposed to be found by name lookup (rather, we see them through the
ObjCInterfaceDecl).

Adding another namespace will work, but I don't know if it's the best
answer. Perhaps what we really mean is that ObjCImplementationDecls
really don't have names at all: they're just unnamed entities that
attach to other entities, and the only reason they have a name in the
source code is to tie them to the appropriate interface:

@interface Blah
@end

@implementation Blah /* ties this ObjCImplementationDecl to the
ObjCInterfaceDecl above */
@end

One could certainly ask an ObjCImplementationDecl what the name of its
corresponding interface is, through a separate operation, even though
the ObjCImplementationDecl itself is unnamed.

I'm not quite sure which way to go with this. IDNS_NoLookup works, but
I think an unnamed ObjCImplementationDecl will be better. Let's see if
someone with a better understanding of Objective-C semantics can chime
in to see if my understanding of ObjCImplementationDecl is reasonable.

There are couple of situations that @implementation is not tied to its interface;
  case of an @implementation without an interface
case of a category @implementation which ties it to its category even though
there must be an interface as well. But this case need go though ObjCCategoryDecl
(which then has the ObjCInterfaceDecl). There might be other corner cases that I cannot
recall at this time.

- Fariborz

7

For everyone who doesn't watch commits: we've gone slightly further this with, removing ScopedDecl entirely and collapsing its fields into Decl so that *every* declaration lives within a DeclContext. That way, traversing through the various DeclContexts in a program will (eventually) visit every declaration in the program.

Many *Decl nodes inherit from NamedDecl, meaning that they can have a name and therefore can be found by name lookup. To address the problem that Piotr found, we've taken some *Decl nodes that used to be NamedDecls (ObjCImplementationDecl and LinkageSpecDecl, for example) and made them inherit directly from Decl so that they don't ever have names and can't ever be inserted for name lookup to find.

   - Doug