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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
