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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/include/clang/AST/DeclCXX.h:1778-1780
+  void setLambdaNumbering(Decl *ContextDecl, unsigned IndexInContext,
+                          unsigned ManglingNumber,
+                          unsigned DeviceManglingNumber,
----------------
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.


================
Comment at: clang/include/clang/AST/MangleNumberingContext.h:65
+  // sequence number and is not ABI-dependent.
+  unsigned getNextLambdaIndex() { return LambdaIndex++; }
 };
----------------
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.


================
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;
+
----------------
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.]


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:574-576
+  } else if (auto *VD = dyn_cast<VarDecl>(D)) {
+    ReadVarDeclInit(VD);
   }
----------------
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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145737/new/

https://reviews.llvm.org/D145737