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

[Aaron Ballman via cfe-commits]

Oh wow, and noload_lookups isn't implemented directly in terms of
noload_lookup, oof (they both use the same underlying functionality of
the LookupPtr's map, but still).

Okay, I'm fine skipping the testing in this case. Thank you for
entertaining the idea!

~Aaron



On Mon, Apr 8, 2024 at 10:20 PM Chuanqi Xu <chuanqi.xcq at alibaba-inc.com> wrote:
>
> 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
> > > 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