[PATCH] D53708: [ASTImporter] Add importer specific lookup

[Gabor Marton via Phabricator via cfe-commits]

martong added a comment.

In D53708#1332922 <https://reviews.llvm.org/D53708#1332922>, @riccibruno wrote:

> In D53708#1332868 <https://reviews.llvm.org/D53708#1332868>, @martong wrote:
>
> > > This is a perhaps a naive comment, but why is localUncachedLookup used
> > >  instead of noload_lookup ? There is a fixme dating from 2013 stating
> > >  that noload_lookup should be used instead.
> >
> > This is not a naive question. I was wondering myself on the very same thing when I started working on the ASTImporter at 2017. 
> >  I think when `noload_lookup` was introduced the author of that function tried to replace `localUncachedLookup` in ASTImporter, but that broke some tests. The major difference between the two functions is that the latter can find declarations even if they are not stored in the `LookupPtr`. This is rather unusual from the C/C++ point of view of lookup, and not working in all cases (see the introduction of this patch). Ironically, I can't just get rid of `localUncachedLookup` because it would break some LLDB test cases.
>
>
> Ah I think I understand. For my understanding (and please correct me if I am wrong here):
>
> `localUncachedLookup` will first try to do a normal lookup, and if that fails it will try to look in the declarations in the declaration context.
>  Now some declarations (including declarations which are not `NamedDecl` and declarations which matches `shouldBeHidden`)
>  will not be added to the `LookupPtr` (via `makeDeclVisibleInContextWithFlags`) and so will not be found though the normal lookup mechanism. You therefore need to use `localUncachedLookup` to find these.
>
> Does this make sense ?


Exactly. And this seems to be an important feature in ASTImporter, because this makes it possible that we can chain into the redecl chain a newly imported AST node to existing and structurally equivalent nodes. (The existing nodes are found by the lookup and then we iterate over the lookup results searching for structurally equivalent ones.)


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53708/new/

https://reviews.llvm.org/D53708