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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
