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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith added inline comments.


================
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;
----------------
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...)


================
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;
 
----------------
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)


================
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;
----------------
I think this patch should (or does?) implement this FIXME.


================
Comment at: clang/include/clang/Basic/SourceManager.h:265-268
+  /// The filename that is used to access OrigEntry.
+  ///
+  /// FIXME: Remove this once OrigEntry is a FileEntryRef with a stable name.
+  StringRef Filename;
----------------
I think you can delete this field. You're effectively implementing the FIXME.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137304