dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.
LGTM, with a couple of nitpicks. If you'd rather not improve the tests that's
probably fine, since the patch doesn't make them any worse.
================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:71
auto &SM = Clang->getSourceManager();
- auto Entry = SM.getFileManager().getFile(Filename);
+ auto Entry = SM.getFileManager().getOptionalFileRef(Filename);
EXPECT_TRUE(Entry);
----------------
Test might be cleaner with `EXPECT_THAT_ERROR`; see comment in
ParsedASTTests.cpp.
================
Comment at: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp:564-565
IncludeStructure Includes = ExpectedAST.getIncludeStructure();
- auto MainFE = FM.getFile(testPath("foo.cpp"));
+ auto MainFE = FM.getOptionalFileRef(testPath("foo.cpp"));
ASSERT_TRUE(MainFE);
auto MainID = Includes.getOrCreateID(*MainFE);
----------------
It might be nice to see the errors here on failures. You could do that with:
```
lang=c++
Optional<FileEntryRef> MainFE;
ASSERT_THAT_ERROR(FM.getFileRef(testPath("foo.cpp")).moveInto(MainFE),
Succeeded());
```
The `{EXPECT,ASSERT}_THAT_ERROR` live in `llvm/Testing/Support/Error.h`.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:6048-6049
if (!FullFileName.empty())
- if (auto FE = PP.getFileManager().getFile(FullFileName))
+ if (auto FE = PP.getFileManager().getOptionalFileRef(FullFileName))
File = *FE;
----------------
I think you can remove the inner `if` here.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D123574/new/
https://reviews.llvm.org/D123574
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
