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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 accepted this revision.
jansvoboda11 added a comment.
This revision is now accepted and ready to land.

LGTM with a couple of small suggestions.



================
Comment at: clang/include/clang/Basic/SourceManager.h:1062
+    Optional<FileEntryRef> F = sloc.getFile().getContentCache().OrigEntry;
+    return F ? &F->getFileEntry() : nullptr;
   }
----------------
Could you wrap `F` in `OptionalFileEntryRefDegradesToFileEntryPtr` to handle the conversion for you?


================
Comment at: clang/lib/Basic/SourceManager.cpp:402
     // pass that file to ContentCache.
-    llvm::DenseMap<const FileEntry *, const FileEntry *>::iterator
-        overI = OverriddenFilesInfo->OverriddenFiles.find(FileEnt);
+    llvm::DenseMap<const FileEntry *, FileEntryRef>::iterator overI =
+        OverriddenFilesInfo->OverriddenFiles.find(FileEnt);
----------------
Nit: Could use `auto`.


================
Comment at: clang/lib/Lex/ModuleMap.cpp:628
+        InferredModuleAllowedBy[Result] =
+            UmbrellaModuleMap ? *UmbrellaModuleMap : nullptr;
         Result->IsInferred = true;
----------------
Nit: Use `OptionalFileEntryRefDegradesToFileEntryPtr`?


================
Comment at: clang/lib/Lex/ModuleMap.cpp:647
+      InferredModuleAllowedBy[Result] =
+          UmbrellaModuleMap ? *UmbrellaModuleMap : nullptr;
       Result->IsInferred = true;
----------------
Nit: Use `OptionalFileEntryRefDegradesToFileEntryPtr`?


================
Comment at: clang/lib/Lex/ModuleMap.cpp:1030
+    Optional<FileEntryRef> ModuleMapRef = getModuleMapFileForUniquing(Parent);
+    ModuleMapFile = ModuleMapRef ? *ModuleMapRef : nullptr;
+  }
----------------
Nit: Use `OptionalFileEntryRefDegradesToFileEntryPtr`? Maybe we should return that from `getModuleMapFileForUniquing` in the first place.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135220