[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

ayermolo added a comment. @modocache Just checking is this diff abandoned? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58920/new/ https://reviews.llvm.org/D58920 ___ cfe-commits mailing list cfe-commit

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

urnathan added inline comments. Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:3198-3199 // lookups will find it. MergeDC->makeDeclVisibleInContextImpl(New, /*Internal*/true); +if (isa(New) && Name.getAsString() == "std") + if (!Reader.getSema()->StdName

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

modocache updated this revision to Diff 227808. modocache added a comment. Rebasing onto the monorepo. @rsmith, I confirmed the test cases that this diff adds still fail on trunk, and that the Clang source changes made in this diff fix the test case failures. Tomorrow I plan on poking around to

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

modocache added a comment. @rsmith, what do you think of the patch as-is? Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58920/new/ https://reviews.llvm.org/D58920 ___ cfe-commits mailing list cfe-commits@lists.llvm.o

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

modocache updated this revision to Diff 200518. modocache added a comment. Hmm... alright, I'm not really sure how I could implement a test that fails without this, but I added a check in the FindExistingResult destructor. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

rsmith added a comment. In D58920#1503872 , @modocache wrote: > Thanks for the help, @rsmith! Your suggestions were spot-on. (It took me a > little while to figure out why, even using the `LazyDeclPtr` directly, I was > still triggering deserialization.

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

modocache updated this revision to Diff 199709. modocache added a comment. Oops, sent the patch from the wrong repository. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58920/new/ https://reviews.llvm.org/D58920 Files: lib/Serialization/ASTReaderDecl.cpp tes

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

modocache updated this revision to Diff 199708. modocache added a comment. Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Thanks for the help, @rsmith! Your suggestions were spot-on. (It took me a little while to figure out why, even using the `LazyDeclPtr` directly, I was

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

rsmith added a comment. The problem may well be that you're recursively triggering deserialization of the `std` namespace from its own deserialization. In D58920#1500246 , @modocache wrote: > @@ -3401,6 +3402,22 @@ ASTDeclReader::FindExistingResult >

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

modocache added a comment. Thanks @rsmith for the guidance here! I appreciate it very much. One snag I ran into after following your suggestion, though, is that when I modify `ASTDeclReader::findExisting` to return Sema's existing implicit std namespace, I run into an assertion later on, when t

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

rsmith added a comment. I expect that we get this wrong in the same way for all the `SemaDeclRefs` declarations (`StdNamespace`, `StdBadAlloc`, and `StdAlignValT`). I think the place where this is going wrong is `ASTDeclReader::findExisting`, which is assuming that the prior declaration for nor

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

andrewjcg added a comment. Sorry for the delay. Just catching up on the code this covers, so apologies if the questions don't make sense. Comment at: lib/Sema/SemaDeclCXX.cpp:9214-9215 getStdNamespace()->setImplicit(true); +if (getLangOpts().Modules) + getStdNam

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

modocache added a comment. I realize this isn't the correct solution, but would any would-be reviewers like to comment on the problem? Whether it's here or on the Bugzilla report https://bugs.llvm.org/show_bug.cgi?id=39287, as a newcomer to Clang modules I could use some help understanding whet

[PATCH] D58920: [Modules][PR39287] Consolidate multiple std's

modocache created this revision. modocache added reviewers: rsmith, andrewjcg. Herald added a project: clang. This patch addresses https://bugs.llvm.org/show_bug.cgi?id=39287. Clang creates a 'std' namespace in one of two ways: either it parses a `namespace std { ... }` declaration, or it creates