[PATCH] D127663: [clang][lex] NFCI: Use DirectoryEntryRef in HeaderSearch::LookupFile

[Ben Barham via Phabricator via cfe-commits]

bnbarham accepted this revision.
bnbarham added inline comments.


================
Comment at: clang/lib/Frontend/FrontendAction.cpp:811
       // Relative searches begin from CWD.
-      const DirectoryEntry *Dir = nullptr;
-      if (auto DirOrErr = CI.getFileManager().getDirectory("."))
-        Dir = *DirOrErr;
-      SmallVector<std::pair<const FileEntry *, const DirectoryEntry *>, 1> CWD;
-      CWD.push_back({nullptr, Dir});
+      auto Dir = CI.getFileManager().getOptionalDirectoryRef(".");
+      SmallVector<std::pair<const FileEntry *, DirectoryEntryRef>, 1> CWD;
----------------
jansvoboda11 wrote:
> bnbarham wrote:
> > Worth adding an assert here? It'd be nice to cleanup the CWD handling in FileManager at some point.
> Assert that `Dir` is not empty? Seems overly defensive to me, what would be the benefit of that? Catching if the working directory was yanked from underneath Clang?
Not sure what I was thinking here - the assertion itself would be pointless, since `*Dir` will assert non-empty anyway. I imagine this was more about how we're grabbing an optional and not checking it, which IMO is often an anti pattern. What do you want to happen if it is somehow empty? Couldn't we just not do the `push_back`?

Also could use `emplace_back(nullptr, *Dir)` instead, not that it really matters.


================
Comment at: clang/lib/Lex/PPDirectives.cpp:891
   // stack, record the parent #includes.
-  SmallVector<std::pair<const FileEntry *, const DirectoryEntry *>, 16>
-      Includers;
+  SmallVector<std::pair<const FileEntry *, DirectoryEntryRef>, 16> Includers;
   bool BuildSystemModule = false;
----------------
jansvoboda11 wrote:
> bnbarham wrote:
> > Could we change this to a FileEntryRef as well, or would you prefer to do that later? It'll touch basically all the same places again (eg. each parameter above).
> I'd prefer to do it separately for smaller diff and easier bisection. LMK if you have strong preference for doing it in the same commit.
Not really, just seemed like it would hit all the same lines to me so it'd be roughly the same in terms of diff size. Up to you.


================
Comment at: clang/test/Modules/filename.cpp:2
+// RUN: rm -rf %t
+// RUN: split-file %s %t
 
----------------
So much nicer with split-file, thanks for updating!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127663