[PATCH] D137304: [clang] Store filename per include instead of mutating filename

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir added a comment.

Thanks for the timely review!

> the DenseMaps that need this unique-by-FileEntry* behaviour should probably be the ones using a custom DenseMapInfo.

Yeah, that's the way I'm currently leaning.  Make the default for `DenseMap` and `==` be path equality and have a separate `DenseMapInfo` and `isSameFileEntry` for cases that want it.  Thankfully there seems to be very little code depending on either of these. Though if I take your suggestion for `FileInfos` it will avoid needing a `DenseMap<FileInfoRef` in this patch specifically.



================
Comment at: clang/include/clang/Basic/SourceManager.h:135-139
   /// References the file which the contents were actually loaded from.
   ///
   /// Can be different from 'Entry' if we overridden the contents of one file
   /// with the contents of another file.
   const FileEntry *ContentsEntry;
----------------
dexonsmith wrote:
> It's possible this could be factored out as a follow-up, and/or moved up to the Named ContentCache level. (Not sure... asking a question really...)
I think this belongs here. It is used to lazily get the contents of the file (see the getBuffer functions).


================
Comment at: clang/include/clang/Basic/SourceManager.h:147-163
   /// Indicates whether the buffer itself was provided to override
   /// the actual file contents.
   ///
   /// When true, the original entry may be a virtual file that does not
   /// exist.
   unsigned BufferOverridden : 1;
 
----------------
dexonsmith wrote:
> Have you thought about whether it makes sense for these fields to be shared for all `FileEntryRef`s to the same `FileEntry`?  (Maybe it does! just checking)
Yes, I think this split actually makes the behaviour clearer. The remaining fields are related to the contents, while the extracted fields are for the name.  You only need one SourceLineCache, etc. for one set of file contents.


================
Comment at: clang/include/clang/Basic/SourceManager.h:261-263
+  /// FIXME: Make non-optional using a virtual file as needed, remove \c
+  /// Filename and use \c OrigEntry.getNameAsRequested() instead.
+  OptionalFileEntryRefDegradesToFileEntryPtr OrigEntry;
----------------
dexonsmith wrote:
> I think this patch should (or does?) implement this FIXME.
I think it can still be None/nullptr for some buffers.  I'll verify that.  IIRC the trick to removing Filename is to create virtual file refs in some places that currently don't have one.


================
Comment at: clang/include/clang/Basic/SourceManager.h:652-662
+  llvm::DenseMap<FileEntryRef, SrcMgr::NamedContentCache *,
+                 FileEntryRefSameRefDenseMapInfo>
+      NamedFileInfos;
+
   /// Memoized information about all of the files tracked by this
   /// SourceManager.
   ///
----------------
dexonsmith wrote:
> It feels expensive to have both of these maps. I wonder if instead we could do something like:
> 
> ```
> lang=c++
> DenseMap<const FileEntry*, TinyPtrSet<NamedContentCache*>> FirstFileInfo;
> ```
> I.e., lookup by `FileEntry*`, then linear search for the right `FileEntryRef`.
> 
> Similar idea: just point at `NamedContentCache*` directly here, but add `NamedContentCache *NamedContentCache::Next` to turn it into a linked list. Also lookup by `FileEntry*` and then a linear search for the right ref.
> 
> Note that `FileInfo` itself would have a direct pointer to the right thing; no need for a linear search.
Yeah, the `TinyPtrSet` approach here is something I thought about. I started with the two `DenseMap`s since it's simpler, but I can look at this again. Probably either of your suggestions are faster in practice, even if they have a worse worst case in theory, though I'll try to confirm that before committing to one or the other.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137304/new/

https://reviews.llvm.org/D137304