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

[Aaron Ballman via Phabricator via cfe-commits]

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