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