[PATCH] D135220: [clang] Update ModuleMap::getModuleMapFile* to use FileEntryRef

[Ben Barham via Phabricator via cfe-commits]

bnbarham accepted this revision.
bnbarham added inline comments.


================
Comment at: clang/include/clang/Basic/SourceManager.h:146-155
   /// 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;
 
   /// The filename that is used to access OrigEntry.
----------------
I haven't checked all uses, but do we still need all of `OrigEntry`/`ContentsEntry`/`Filename`? Seems like a single `FileEntryRef` should encapsulate all the information we need there.


================
Comment at: clang/include/clang/Basic/SourceManager.h:1062
+    Optional<FileEntryRef> F = sloc.getFile().getContentCache().OrigEntry;
+    return F ? &F->getFileEntry() : nullptr;
   }
----------------
jansvoboda11 wrote:
> Could you wrap `F` in `OptionalFileEntryRefDegradesToFileEntryPtr` to handle the conversion for you?
`OrigEntry` is a `OptionalFileEntryRefDegradesToFileEntryPtr`, so this should just be able to be `return slot.getFile().getContentCache().OrigEntry`?


================
Comment at: clang/lib/Lex/ModuleMap.cpp:1030
+    Optional<FileEntryRef> ModuleMapRef = getModuleMapFileForUniquing(Parent);
+    ModuleMapFile = ModuleMapRef ? *ModuleMapRef : nullptr;
+  }
----------------
jansvoboda11 wrote:
> Nit: Use `OptionalFileEntryRefDegradesToFileEntryPtr`? Maybe we should return that from `getModuleMapFileForUniquing` in the first place.
I like the idea of returning `OptionalFileEntryRefDegradesToFileEntryPtr` until we move all this to use `FileEntryRef` as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135220