[PATCH] D89749: SourceManager: Don't allocate an SLocEntry until it's loaded

Mon Oct 26 09:35:58 PDT 2020

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