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

[Ben Langmuir via Phabricator via cfe-commits]

benlangmuir 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.
----------------
bnbarham wrote:
> 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.
Yes, see my change to the FIXME above. Before we can get rid of Filename, `OrigEntry` has to be non-Optional, which means the code that works with virtual buffers needs to use a virtual file as well.  I don't think it's a huge deal, but it seemed orthogonal from the rest of this change.  `ContentsEntry` is unrelated, as it is not used for its filename only for its contents.


================
Comment at: clang/include/clang/Basic/SourceManager.h:1062
+    Optional<FileEntryRef> F = sloc.getFile().getContentCache().OrigEntry;
+    return F ? &F->getFileEntry() : nullptr;
   }
----------------
bnbarham wrote:
> 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`?
Correct, this was just a leftover from before I decided to make OrigEntry use `OptionalFileEntryRefDegradesToFileEntryPtr`. 


================
Comment at: clang/lib/Lex/ModuleMap.cpp:1030
+    Optional<FileEntryRef> ModuleMapRef = getModuleMapFileForUniquing(Parent);
+    ModuleMapFile = ModuleMapRef ? *ModuleMapRef : nullptr;
+  }
----------------
bnbarham wrote:
> 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.
I would prefer to keep returning `Optional<FileEntryRef>`.  My reasoning is that `OptionalFileEntryRefDegradesToFileEntryPtr` should only be used on an API like this function when it would otherwise cause a lot of unnecessary workarounds at the callsites.  For `ContentCache::OrigEntry`, for example, that is clearly the case.  For `getModuleMapFileForUniquing` in my latest patch there are 11 total calls to this function, and only two of them would be improved by `OptionalFileEntryRefDegradesToFileEntryPtr`.  On balance, think it's better to just put the degrading pointer at those 2 callsites and keep the API itself clean. 


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

https://reviews.llvm.org/D135220