[PATCH] D140002: [C++20] [Modules] Merging of lambda types in deserialization

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added a comment.

In D140002#4000113 <https://reviews.llvm.org/D140002#4000113>, @vsapsai wrote:

> This is my first and pretty shallow review pass. Need to read the standard more thoroughly to be more useful. Others are welcome to chime in (and as I'll be on vacation are encouraged to chime in).

Thanks for reviewing this!

> What do you mean by
>
>> Due to we don't care about merging unnamed entities before, now it is pretty problematic to fix it in a well formed.
>
> In ASTReaderDecl.cpp we have a bunch of code to deal with anonymous decls, that's why I'm asking. Though I haven't tried to use any of that for your current problem.

Oh, this is not accurate. I wanted to say lambdas instead of anonymous decls. In `ASTDeclReader::findExisting`, lambdas will fall in the block: https://github.com/llvm/llvm-project/blob/bcd0bf9284db0d5d4697611ff9bf3243504aab07/clang/lib/Serialization/ASTReaderDecl.cpp#L3269-L3276. Since lambdas has no name and
`serialization::needsAnonymousDeclarationNumber(NamedDecl*)` will return false for lambdas which are no defined in functions: `https://github.com/llvm/llvm-project/blob/bcd0bf9284db0d5d4697611ff9bf3243504aab07/clang/lib/Serialization/ASTCommon.cpp#L470-L485`

So we can't find existing decls for lambdas so we can't merge them. This is what I wanted to say. And now I delete it since it may be confusing.

> Few stylistic nits:
>
> - most of tests in "clang/test/Modules" start with a lowercase and use kebab-naming-style (sometimes with underscores);
> - can you please use more descriptive names in tests? Currently hunting for 1-character difference isn't fun and doesn't convey the purpose and intention of the test;
> - is it possible to reuse some of the testing code because trying to find differences and what they mean is time-consuming.

Thanks for this. Let's do it after we get a consensus in the higher level.

> Looks like not all branches in the added code are covered by tests but I need more time to play with the code.

Oh, I didn't test the coverage when developing. I'll check it when I look at this again.

> Test case MergeLambdas3.cppm is unstable. Sometimes it passes but usually it fails.

Weird. What is the error message if it fails?



================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:2161-2198
+  /// FIXME: The `DeclContext::noload_lookup()` wouldn't load local lexical
+  /// lookups since unnamed declarations are skipped. We can find this in
+  /// `DeclContext::buildLookupImpl` and `shouldBeHidden(NamedDecl*)` in
+  /// DeclBase.cpp. So the `DeclContext::noload_lookup()` here can only find
+  /// decls during the deserilizations. For example:
+  ///
+  /// ```
----------------
vsapsai wrote:
> ChuanqiXu wrote:
> > The comment tells why this is a workaround and what we need to do to fix the problem properly.
> Is there are a test covering this workaround? I've removed it and see no new test failures.
Weird. MergeLambdas3.cppm should cover this. Let me try a double check.


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:2222
+  if (TagDecl *Existing = findExistingLambda(D))
+    mergeRedeclarable(D, Existing, Redecl);
+}
----------------
vsapsai wrote:
> How does this `mergeRedeclarable` work with other `mergeRedeclarable` calls in `ASTDeclReader::VisitCXXRecordDeclImpl`? Are multiple calls OK, do you expect it never to happen or something else?
The original `mergeRedeclarable` in `ASTDeclReader::VisitCXXRecordDeclImpl` are the mergeRedeclarable with two parameters and it will call the `mergeRedeclarable ` with 3 parameters if it found an existing declaration. So the relationship of the `mergeLambda ` here and the `mergeRedeclarable ` in  `ASTDeclReader::VisitCXXRecordDeclImpl` is that they find different things and they will call the same method.

The original `mergeRedeclarable`  in `ASTDeclReader::VisitCXXRecordDeclImpl` can't find existing lambdas for the reason I explained in the main thread. 

And `mergeLambda` is called after we read the definition about lambdas and the original `mergeRedeclarable`  in `ASTDeclReader::VisitCXXRecordDeclImpl` is called before we read the definitions since it mainly cares about the declaration instead of the definition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140002