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