vsapsai added a comment.

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).

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.

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.

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



================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:2222
+  if (TagDecl *Existing = findExistingLambda(D))
+    mergeRedeclarable(D, Existing, Redecl);
+}
----------------
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?


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

Reply via email to