[PATCH] D123771: [clang][lex] NFCI: Use DirectoryEntryRef in HeaderSearch::load*()

[Ben Barham via Phabricator via cfe-commits]

bnbarham added inline comments.


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:339
     // Search for a module map file in this directory.
-    if (loadModuleMapFile(Dir.getDir(), IsSystem,
+    if (loadModuleMapFile(*Dir.getDirRef(), IsSystem,
                           /*IsFramework*/false) == LMM_NewlyLoaded) {
----------------
I'd prefer the following since I had to specifically go and check `getDirRef`, though I suppose I probably would have done so even with a comment 😅 :
```
// Only returns None if not a normal directory, which we just checked
DirectoryEntryRef NormalDir = *Dir.getDirRef();
...loadModuleMapFile(NormalDir, ...
```


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1583
+        ::getTopFrameworkDir(FileMgr, FrameworkName, SubmodulePath);
+    assert(TopFrameworkDir && "Could not find the top-most framework dir");
 
----------------
Can we just return false in this case, or is it definitely always a logic error?


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1630
+              llvm::sys::path::parent_path(OriginalModuleMapFile))) {
+        Dir = *MaybeDir;
       } else {
----------------
Shouldn't need the * should it? Also doesn't seem necessary to actually check here since `Dir` hasn't been set yet. Just `Dir = FileMgr.getOptionalDirectoryRef...` should be fine.


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1636
     } else {
-      Dir = File->getDir();
+      Dir = File->getLastRef().getDir();
     }
----------------
I assume this is the place you mentioned in the message? (ie. to prevent patch from growing even more)


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1639
 
     StringRef DirName(Dir->getName());
     if (llvm::sys::path::filename(DirName) == "Modules") {
----------------
Can't `Dir` still be `None` here? It *shouldn't* be since the directory for `OriginalModuleMapFile` should exist, but I'd feel more comfortable with either an early return or an assert here.

Same thing below at the switch as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123771