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