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

[Jan Svoboda via Phabricator via cfe-commits]

jansvoboda11 marked 5 inline comments as done.
jansvoboda11 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) {
----------------
bnbarham wrote:
> 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, ...
> ```
Fair enough.


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1583
+        ::getTopFrameworkDir(FileMgr, FrameworkName, SubmodulePath);
+    assert(TopFrameworkDir && "Could not find the top-most framework dir");
 
----------------
bnbarham wrote:
> Can we just return false in this case, or is it definitely always a logic error?
I agree returning `false` here would be better, but I wanted to keep this patch as NFC as possible. How about I make that change in a follow-up patch?


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1630
+              llvm::sys::path::parent_path(OriginalModuleMapFile))) {
+        Dir = *MaybeDir;
       } else {
----------------
bnbarham wrote:
> 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.
Nice catch!


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


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1639
 
     StringRef DirName(Dir->getName());
     if (llvm::sys::path::filename(DirName) == "Modules") {
----------------
bnbarham wrote:
> 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.
I think `Dir` can't be `None` at this point - adding an assertion here sounds like a good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123771