teemperor added a comment. >> 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.
I'm actually not even sure if we have a nice framework where we could test modules + ASTImporter, so don't feel obligated to solve this as part of this patch. >> 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>`. Thanks! >> 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. True. Let's keep the 0 sentinel value for now then, I don't think the small advantage of using 2^32 is worth that trouble. >> 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
