Re: [clang] 468dc32 - [NFC] Make `DeclContext::noload_lookup()` accept transparent context

[Chuanqi Xu via cfe-commits]

Hi Aaron,
 If I didn't miss anything, the function used in `ASTDumper.cpp` is `noload_lookups` instead of `noload_lookup`. So maybe that wouldn't work.
 And I checked quickly against the code, it looks like all the existing use of `noload_lookup` has already ensured that the decl context wouldn't be transparent context. So I feel like we can only add unit tests for it if we want.
Thanks,
Chuanqi
------------------------------------------------------------------
From:Aaron Ballman <aaron at aaronballman.com>
Send Time:2024 Apr. 8 (Mon.) 20:20
To:Chuanqi<chuanqi.xcq at alibaba-inc.com>
Cc:Chuanqi Xu<yedeng.yd at linux.alibaba.com>; "cfe-commits"<cfe-commits at lists.llvm.org>
Subject:Re: [clang] 468dc32 - [NFC] Make `DeclContext::noload_lookup()` accept transparent context
I was thinking we could use the AST dumper to test this via lit
because we have `-ast-dump-lookups` which uses `noload_lookup`. Do you
think that'd work?
~Aaron
On Sat, Apr 6, 2024 at 9:59 PM Chuanqi Xu <chuanqi.xcq at alibaba-inc.com> wrote:
>
> Hi Aaron,
>
> This was found in a working in progress local patch that I was using "noload_lookup". Since `noload_lookup` is relatively rarely used in the code base, I assume we can't find a LIT test case that was previously asserted.
>
> I can add a unittest if you want. WDYT?
>
> Thanks,
> Chuanqi
>
> ------------------------------------------------------------------
> From:Aaron Ballman <aaron at aaronballman.com>
> Send Time:2024 Apr. 3 (Wed.) 19:56
> To:Chuanqi Xu<yedeng.yd at linux.alibaba.com>
> Cc:"cfe-commits"<cfe-commits at lists.llvm.org>
> Subject:Re: [clang] 468dc32 - [NFC] Make `DeclContext::noload_lookup()` accept transparent context
>
> I'm not opposed to the changes, but they do seem like functional
> changes -- we used to trigger an assertion and now we're swallowing it
> and changing the behavior; can you add a test case that would have
> previously asserted?
>
> ~Aaron
>
> On Wed, Apr 3, 2024 at 3:05 AM Chuanqi Xu via cfe-commits
> <cfe-commits at lists.llvm.org> wrote:
> >
> >
> > Author: Chuanqi Xu
> > Date: 2024-04-03T15:03:07+08:00
> > New Revision: 468dc32ff55d19f55132cbcc4d6ceb1f6d1c12cf
> >
> > URL: https://github.com/llvm/llvm-project/commit/468dc32ff55d19f55132cbcc4d6ceb1f6d1c12cf <https://github.com/llvm/llvm-project/commit/468dc32ff55d19f55132cbcc4d6ceb1f6d1c12cf >
> > DIFF: https://github.com/llvm/llvm-project/commit/468dc32ff55d19f55132cbcc4d6ceb1f6d1c12cf.diff <https://github.com/llvm/llvm-project/commit/468dc32ff55d19f55132cbcc4d6ceb1f6d1c12cf.diff >
> >
> > LOG: [NFC] Make `DeclContext::noload_lookup()` accept transparent context
> >
> > Now the `DeclContext::noload_lookup()` asserts that 'this' is not a
> > transparent context. However, this is not consistent with
> > `DeclContext::lookup()`, which will lookup into its parent context if
> > 'this' is a transparent context.
> >
> > This patch makes the behavior of `DeclContext::noload_lookup()` to be
> > consistent with `DeclContext::lookup()`, to lookup into the parent
> > context if 'this' is a transparent context.
> >
> > Added:
> >
> >
> > Modified:
> > clang/lib/AST/DeclBase.cpp
> >
> > Removed:
> >
> >
> >
> > ################################################################################
> > diff --git a/clang/lib/AST/DeclBase.cpp b/clang/lib/AST/DeclBase.cpp
> > index 2cbb86b31b5e2e..66a727d9dd0c39 100644
> > --- a/clang/lib/AST/DeclBase.cpp
> > +++ b/clang/lib/AST/DeclBase.cpp
> > @@ -1852,9 +1852,9 @@ DeclContext::lookup(DeclarationName Name) const {
> >
> > DeclContext::lookup_result
> > DeclContext::noload_lookup(DeclarationName Name) {
> > - assert(getDeclKind() != Decl::LinkageSpec &&
> > - getDeclKind() != Decl::Export &&
> > - "should not perform lookups into transparent contexts");
> > + // For transparent DeclContext, we should lookup in their enclosing context.
> > + if (getDeclKind() == Decl::LinkageSpec || getDeclKind() == Decl::Export)
> > + return getParent()->noload_lookup(Name);
> >
> > DeclContext *PrimaryContext = getPrimaryContext();
> > if (PrimaryContext != this)
> >
> >
> >
> > _______________________________________________
> > cfe-commits mailing list
> > cfe-commits at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits <https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20240409/05cb04ac/attachment.html>