[PATCH] D116792: [AST] lookup in parent DeclContext for transparent DeclContext

Mon Jan 10 06:17:08 PST 2022

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

In D116792#3230478 <https://reviews.llvm.org/D116792#3230478>, @ChuanqiXu wrote:

> In D116792#3227379 <https://reviews.llvm.org/D116792#3227379>, @erichkeane wrote:
>
>> I had to do something similar for this at one point: https://github.com/llvm/llvm-project/commit/90010c2e1d60c6a9a4a0b30a113d4dae2b7214eb
>>
>> I seem to remember hitting this assert, and from my end, I think I decided even calling 'lookup' with the linkage spec to be a mistake (BTW, you might consider updating that 'Encloses' for 'export' as well!).
>
> Yeah, it is another bug for 'export'. I've tried to address it in https://reviews.llvm.org/D116911 with the same style.
>
>> Is there any mechanism in the parent call of 'lookup' here to make it get the right thing?
>
> And 'lookup' is called in various places. For example, from the stack trace of the crash, we could find that the parent of call is `DeclareImplicitDeductionGuides`. And I tried to handle it in  `DeclareImplicitDeductionGuides`, then the compiler would crash again at `LookupDirect` in `SemaLookup.cpp`. I feel it is not good to add checks for places to call `lookup`. I agree that it is odd to lookup in a transparent DeclContext. But I feel it is not bad to lookup in the enclosing context from the definition of transparent DeclContext. Any thoughts?

Hmm... I didn't realize this was the root 'lookup' function, and thank you for the above analysis.  It seems to make more sense to me as well to have this be tolerant of this lookup setup.  So LGTM.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116792