aaron.ballman added inline comments.
================
Comment at: clang/include/clang/AST/DeclCXX.h:1778-1780
+ void setLambdaNumbering(Decl *ContextDecl, unsigned IndexInContext,
+ unsigned ManglingNumber,
+ unsigned DeviceManglingNumber,
----------------
rsmith wrote:
> aaron.ballman wrote:
> > My kingdom for a strong typedef so we can differentiate between the various
> > indices here within the type system. (It probably isn't worth the extra
> > effort, but I do have some slight concerns about how easy it is to
> > accidentally pass the wrong number.)
> I've moved `Sema::LambdaNumbering` here and used it more broadly, which
> should help us avoid errors from getting the argument order wrong.
Great idea, thank you!
================
Comment at: clang/include/clang/AST/MangleNumberingContext.h:65
+ // sequence number and is not ABI-dependent.
+ unsigned getNextLambdaIndex() { return LambdaIndex++; }
};
----------------
rsmith wrote:
> aaron.ballman wrote:
> > Does the index `0` mean "perhaps not-yet-assigned" when deserializing?
> > Assuming I have that correct, perhaps we want to return `++LambdaIndex`
> > instead?
> `0` has no special meaning. Every lambda that we parse locally gets an index
> assigned when we give it a context declaration, and every lambda that we
> import has this field filled in when the struct containing the field is
> loaded. There's no window where the lambda has a context declaration but
> doesn't have a numbering within that context.
Thank you for the clarification!
================
Comment at: clang/lib/AST/ASTContext.cpp:6724-6725
+ // their types. Assume the types will end up being the same.
+ if (VarX->getType().isNull() || VarY->getType().isNull())
+ return true;
+
----------------
rsmith wrote:
> aaron.ballman wrote:
> > Do we have to worry about the case where this function is not called during
> > deserialization, but some other time when the variable had an error
> > regarding its type? Or do we recover in that situation (usually with an int
> > type) sufficiently that this shouldn't result in surprises?
> The type being null is a violation of AST invariants, so that should never be
> observed here except while deserializing.
>
> [It's unfortunate that this function got moved out of the AST reader. That's
> inviting bugs, because deserialization can't rely on AST invariants holding,
> and in particular can't call arbitrary AST functions. I've added some words
> of caution at the start of this function at least, but we should look at
> whether we can move this back into the AST reader. It looks like there aren't
> many dependencies on it and they're only using a very small part of the
> functionality here.]
> The type being null is a violation of AST invariants, so that should never be
> observed here except while deserializing.
Okay, I thought that was the case, but I think there's one more time when we
could observe it, and that's when calling this function from a debugger, but,
that's YOLO mode and nothing we need to worry about here.
Good point about moving this back into the AST reader, I had not realized how
fragile this API was.
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:574-576
+ } else if (auto *VD = dyn_cast<VarDecl>(D)) {
+ ReadVarDeclInit(VD);
}
----------------
rsmith wrote:
> aaron.ballman wrote:
> > Do we need to do anything for structured bindings?
> Hmm. Great question ;-)
>
> So, the type of a `BindingDecl` can involve a lambda closure type, and the
> `BindingDecl` will be deserialized with the `DecompositionDecl`, before we
> had a chance to merge the lambda. Eg:
>
> ```
> template<typename T> struct wrap {
> T v;
> };
> inline auto [x] = wrap{[]{}};
> ```
>
> ... won't merge properly across modules.
>
> This is probably not hard to fix -- I think we can defer initializing the
> mapping from a `DecompositionDecl` to its `BindingDecl`s until we've
> otherwise finished deserialization, and maybe we could make the `BindingDecl`
> pointers in a `DecompositionDecl` be lazy pointers for good measure. But this
> patch is already pretty big; mind if I do that as a follow-up change?
> But this patch is already pretty big; mind if I do that as a follow-up change?
Totally fine to do in a follow-up, thank you!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D145737/new/
https://reviews.llvm.org/D145737
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
