aaron.ballman added a comment.
Thank you for working on this Richard, that is a pretty subtle issue! I'm still
wrapping my head around the changes (which mostly look reasonable to me), but I
did have a few questions I ran into.
================
Comment at: clang/include/clang/AST/DeclCXX.h:1778-1780
+ void setLambdaNumbering(Decl *ContextDecl, unsigned IndexInContext,
+ unsigned ManglingNumber,
+ unsigned DeviceManglingNumber,
----------------
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.)
================
Comment at: clang/include/clang/AST/ExternalASTSource.h:387
+ // Ensure the integer is in pointer form.
+ get(Source);
+ return reinterpret_cast<T**>(&Ptr);
----------------
I think this helps make it clear that the function is only being called for its
side effects.
================
Comment at: clang/include/clang/AST/MangleNumberingContext.h:65
+ // sequence number and is not ABI-dependent.
+ unsigned getNextLambdaIndex() { return LambdaIndex++; }
};
----------------
Does the index `0` mean "perhaps not-yet-assigned" when deserializing? Assuming
I have that correct, perhaps we want to return `++LambdaIndex` instead?
================
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;
+
----------------
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?
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:574-576
+ } else if (auto *VD = dyn_cast<VarDecl>(D)) {
+ ReadVarDeclInit(VD);
}
----------------
Do we need to do anything for structured bindings?
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:876
// function.
- if (isa<FunctionDecl>(VD))
+ if (isa<FunctionDecl>(VD) || isa<VarDecl>(VD))
DeferredTypeID = Record.getGlobalTypeID(Record.readInt());
----------------
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:1604-1608
+ if (HasDeducedType) {
+ Reader.PendingDeducedVarTypes.push_back({VD, DeferredTypeID});
+ } else {
+ VD->setType(Reader.GetType(DeferredTypeID));
+ }
----------------
================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:1967-1969
+ if (unsigned DeviceManglingNumber = Record.readInt()) {
+ Reader.getContext().DeviceLambdaManglingNumbers[D] =
DeviceManglingNumber;
+ }
----------------
================
Comment at: clang/lib/Serialization/ASTWriter.cpp:5330
+ } else if (HasAddedVarDefinition) {
+ const VarDecl *VD = cast<VarDecl>(D);
+ Record.push_back(UPD_CXX_ADDED_VAR_DEFINITION);
----------------
================
Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:299-301
+ if (VarDecl *VD = dyn_cast<VarDecl>(D)) {
+ Record.AddVarDeclInit(VD);
+ }
----------------
================
Comment at: clang/test/Modules/lambda-in-variable.cpp:94
+#endif
+
+static_assert(MultipleLambdas() == 2);
----------------
Should we add a test for structured bindings as well?
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
