[PATCH] D65534: [clang] Change FileManager to use llvm::ErrorOr instead of null on failure

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/ClangTidy.cpp:240
+    auto File = SourceMgr.getFileManager().getFile(FilePath);
+    if (!File)
+      return SourceLocation();
----------------
Previously we'd hit the assert in `translateFile()` called from `getOrCreateFileID()`. This seems like a better way to handle that.


================
Comment at: clang/include/clang/Basic/FileManager.h:217
   ///
-  /// This returns NULL if the file doesn't exist.
+  /// This returns a \c std::error_code if there was an error loading the file.
   ///
----------------
harlanhaskins wrote:
> JDevlieghere wrote:
> > harlanhaskins wrote:
> > > jkorous wrote:
> > > > Does that mean that it's now safe to assume the value is `!= NULL` in the absence of errors?
> > > That's the intent of these changes, yes, but it should also be documented. 👍
> > In line with the previous comment, should we just pass a reference then?
> I'm fine with doing that, but it would introduce a significant amount more churn into this patch.
I feel that this patch is already huge. It's a good idea though - maybe a separate patch?


================
Comment at: clang/lib/ARCMigrate/FileRemapper.cpp:156
+      auto newE = FileMgr->getFile(tempPath);
+      if (newE) {
+        remap(origFE, *newE);
----------------
It seems like we're now avoiding the assert in `remap()` we'd hit previously. Is this fine?


================
Comment at: clang/lib/ARCMigrate/FileRemapper.cpp:229
 const FileEntry *FileRemapper::getOriginalFile(StringRef filePath) {
-  const FileEntry *file = FileMgr->getFile(filePath);
+  const FileEntry *file;
+  if (auto fileOrErr = FileMgr->getFile(filePath))
----------------
Shouldn't we initialize this guy to `nullptr`?


================
Comment at: clang/lib/Basic/FileManager.cpp:73
   if (llvm::sys::path::is_separator(Filename[Filename.size() - 1]))
-    return nullptr; // If Filename is a directory.
+    return std::errc::is_a_directory; // If Filename is a directory.
 
----------------
I think you've made the code self-documenting. Could you please remove the comment?


================
Comment at: clang/lib/Frontend/FrontendAction.cpp:715
       SmallString<128> DirNative;
-      llvm::sys::path::native(PCHDir->getName(), DirNative);
+      if (*PCHDir == nullptr) {
+        llvm::dbgs() << "Directory " << PCHInclude << " was null but no error thrown";
----------------
Could this actually happen? I was expecting the behavior to be consistent with `getFile()`.


================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1448
   if (getHeaderSearchOpts().ModuleMapFileHomeIsCwd)
-    Dir = FileMgr.getDirectory(".");
+    Dir = *FileMgr.getDirectory(".");
   else {
----------------
Seems like we're introducing assert here. Is that fine?


================
Comment at: clang/unittests/Basic/FileManagerTest.cpp:260
+
+  EXPECT_EQ(f1 ? *f1 : nullptr,
+            f2 ? *f2 : nullptr);
----------------
Just an idea - should we compare error codes?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D65534/new/

https://reviews.llvm.org/D65534