dexonsmith planned changes to this revision. dexonsmith added a comment. In D89749#2353651 <https://reviews.llvm.org/D89749#2353651>, @v.g.vassilev wrote:
> Thanks for the patch!! This is a super hot place for us (mostly due to > boost). I will try it on our end and let you know! Great! In D89749#2353602 <https://reviews.llvm.org/D89749#2353602>, @teemperor wrote: > Maybe arc didn't correctly apply the patch, but this causes a few LLDB tests > to fail on my machine: > > lldb-api :: > commands/expression/import-std-module/queue/TestQueueFromStdModule.py > lldb-api :: > commands/expression/import-std-module/list/TestListFromStdModule.py > lldb-api :: > commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py > > From the backtrace this seems like the ASTImporter needs to be updated: Thanks for doing that legwork, I'll take a look. > The tests are just loading a Clang module and then trying to import some > Decls into another ASTContext, so I don't think the error here is specific to > LLDB. We just don't have any ASTImporter tests that exercise modules. I'll add one. > FWIW, if you quickly want to see if your SourceManager changes break LLDB, > then you can just set the LIT_FILTER to "Modules" and run `check-lldb` as > LLDB's use of Clang modules are the only place where we have any valid > SourceLocations within LLDB. This is helpful, thanks. I also need to deal with whatever entitlements `check-lldb` needs (new machine since the last time and I remember it being non-trivial) but I'll figure that out. > 1. The `I - 1` everywhere when translating `SLocEntryLoaded` values to a > `LoadedSLocEntryTable` index is a bit cryptic. [...] Great comment; I think the way I'll do this is to create a simple wrapper type that degrades to `Optional<unsigned>`. > 2. If we have this as an Optional, we might as well make the sentinel value > `std::numeric_limits<unsigned>::max()` instead of 0 in `SLocEntryLoaded`. > Then we can just all values as-is without translating them to an index. Could do, but there's a tradeoff since then a `resize` operation can't/doesn't use zero-initialization. As a result I have a (weak) preference for a 0-sentinel (probably my next patch will keep that semantic), but I'm open to changing it. > 3. Now that the primary purpose of `SLocEntryLoaded` is storing indices and > not the loaded status, maybe something like `SLocEntryIndices` would be a > better name? Good idea. > 4. Do you still have your benchmarking values (or some estimates) for this? > I'm just curious how much memory this actually saves. I'll find some numbers if Vassil hasn't shared his by the time I've fixed the patch. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89749/new/ https://reviews.llvm.org/D89749 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
