[PATCH] D145737: PR60985: Fix merging of lambda closure types across modules.

[Aaron Ballman via Phabricator via cfe-commits]

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