[PATCH] D123574: [clang] NFCI: Use FileEntryRef in PPCallbacks::InclusionDirective

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

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