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

[Harlan Haskins via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL367618: [clang] Change FileManager to use llvm::ErrorOr 
instead of null on failure (authored by harlanhaskins, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D65534?vs=212860&id=212907#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D65534

Files:
  lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
  lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp


Index: 
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
===
--- lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
+++ lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
@@ -247,11 +247,11 @@
 
   bool is_system = true;
   bool is_framework = false;
-  auto *dir =
+  auto dir =
   HS.getFileMgr().getDirectory(module.search_path.GetStringRef());
   if (!dir)
 return error();
-  auto *file = HS.lookupModuleMapFile(dir, is_framework);
+  auto *file = HS.lookupModuleMapFile(*dir, is_framework);
   if (!file)
 return error();
   if (!HS.loadModuleMapFile(file, is_system))
Index: 
lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -908,10 +908,13 @@
   if (file.Write(expr_text, bytes_written).Success()) {
 if (bytes_written == expr_text_len) {
   file.Close();
-  source_mgr.setMainFileID(source_mgr.createFileID(
-  m_compiler->getFileManager().getFile(result_path),
-  SourceLocation(), SrcMgr::C_User));
-  created_main_file = true;
+  if (auto fileEntry =
+  m_compiler->getFileManager().getFile(result_path)) {
+source_mgr.setMainFileID(source_mgr.createFileID(
+*fileEntry,
+SourceLocation(), SrcMgr::C_User));
+created_main_file = true;
+  }
 }
   }
 }


Index: lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
===
--- lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
+++ lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
@@ -247,11 +247,11 @@
 
   bool is_system = true;
   bool is_framework = false;
-  auto *dir =
+  auto dir =
   HS.getFileMgr().getDirectory(module.search_path.GetStringRef());
   if (!dir)
 return error();
-  auto *file = HS.lookupModuleMapFile(dir, is_framework);
+  auto *file = HS.lookupModuleMapFile(*dir, is_framework);
   if (!file)
 return error();
   if (!HS.loadModuleMapFile(file, is_system))
Index: lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/trunk/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -908,10 +908,13 @@
   if (file.Write(expr_text, bytes_written).Success()) {
 if (bytes_written == expr_text_len) {
   file.Close();
-  source_mgr.setMainFileID(source_mgr.createFileID(
-  m_compiler->getFileManager().getFile(result_path),
-  SourceLocation(), SrcMgr::C_User));
-  created_main_file = true;
+  if (auto fileEntry =
+  m_compiler->getFileManager().getFile(result_path)) {
+source_mgr.setMainFileID(source_mgr.createFileID(
+*fileEntry,
+SourceLocation(), SrcMgr::C_User));
+created_main_file = true;
+  }
 }
   }
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jan Korous via Phabricator via cfe-commits]

jkorous accepted this revision.
jkorous marked an inline comment as done.
jkorous added a comment.
This revision is now accepted and ready to land.

LGTM

Thanks for all the work here!




Comment at: clang/lib/ARCMigrate/FileRemapper.cpp:156
+  auto newE = FileMgr->getFile(tempPath);
+  if (newE) {
+remap(origFE, *newE);

harlanhaskins wrote:
> jkorous wrote:
> > It seems like we're now avoiding the assert in `remap()` we'd hit 
> > previously. Is this fine?
> If we want to trap if this temp file fails, then I'm happy removing the 
> condition check. Do you think this should trap on failure or should it check 
> the condition?
To be clear - I didn't have any opinion, just noticed there's a functional 
change and pointed that out.
On the other hand I assume your solution is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65534



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Harlan Haskins via Phabricator via cfe-commits]

harlanhaskins updated this revision to Diff 212860.
harlanhaskins marked 8 inline comments as done.
harlanhaskins added a comment.

Updated in response to feedback


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65534

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp
  clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
  clang-tools-extra/clang-move/Move.cpp
  clang-tools-extra/clang-move/tool/ClangMove.cpp
  clang-tools-extra/clang-reorder-fields/tool/ClangReorderFields.cpp
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clangd/ClangdUnit.cpp
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/modularize/ModularizeUtilities.cpp
  clang/include/clang/Basic/FileManager.h
  clang/lib/ARCMigrate/FileRemapper.cpp
  clang/lib/ARCMigrate/ObjCMT.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/Basic/FileManager.cpp
  clang/lib/Basic/Module.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Frontend/InitHeaderSearch.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/lib/Frontend/Rewrite/FrontendActions.cpp
  clang/lib/Frontend/TextDiagnostic.cpp
  clang/lib/Lex/HeaderMap.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Lex/PPLexerChange.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/GlobalModuleIndex.cpp
  clang/lib/Serialization/ModuleManager.cpp
  clang/lib/Tooling/Core/Replacement.cpp
  clang/lib/Tooling/Refactoring.cpp
  clang/tools/clang-format/ClangFormat.cpp
  clang/tools/clang-import-test/clang-import-test.cpp
  clang/tools/clang-refactor/ClangRefactor.cpp
  clang/tools/clang-refactor/TestSupport.cpp
  clang/tools/clang-rename/ClangRename.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/Indexing.cpp
  clang/unittests/Basic/FileManagerTest.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp
  clang/unittests/Lex/PPCallbacksTest.cpp
  clang/unittests/Tooling/RefactoringTest.cpp
  clang/unittests/Tooling/RewriterTestContext.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
@@ -247,11 +247,11 @@
 
   bool is_system = true;
   bool is_framework = false;
-  auto *dir =
+  auto dir =
   HS.getFileMgr().getDirectory(module.search_path.GetStringRef());
   if (!dir)
 return error();
-  auto *file = HS.lookupModuleMapFile(dir, is_framework);
+  auto *file = HS.lookupModuleMapFile(*dir, is_framework);
   if (!file)
 return error();
   if (!HS.loadModuleMapFile(file, is_system))
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -908,10 +908,13 @@
   if (file.Write(expr_text, bytes_written).Success()) {
 if (bytes_written == expr_text_len) {
   file.Close();
-  source_mgr.setMainFileID(source_mgr.createFileID(
-  m_compiler->getFileManager().getFile(result_path),
-  SourceLocation(), SrcMgr::C_User));
-  created_main_file = true;
+  if (auto fileEntry =
+  m_compiler->getFileManager().getFile(result_path)) {
+source_mgr.setMainFileID(source_mgr.createFileID(
+*fileEntry,
+SourceLocation(), SrcMgr::C_User));
+created_main_file = true;
+  }
 }
   }
 }
Index: clang/unittests/Tooling/RewriterTestContext.h
===
--- clang/unittests/Tooling/RewriterTestContext.h
+++ clang/unittests/Tooling/RewriterTestContext.h
@@ -56,9 +56,9 @@
 llvm::MemoryBuffer::getMemBuffer(Content);
 InMemoryFileSystem->addFile(Name, 0, std::move(Source));
 
-const FileEntry *Entry = Files.getFile(Name);
-assert(Entry != nullptr);
-return Sources.createFileID(Entry, SourceLocation(), SrcMgr::C_User);
+auto Entry = Files.getFile(Name);
+assert(Entry);
+return Sources.createFileID(*Entry, SourceLocation(), SrcMgr::C_User);
   }
 
   // FIXME: this code is mostly a duplicate of
@@ -73,14 +73,14 @@
  

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

[Harlan Haskins via Phabricator via cfe-commits]

harlanhaskins marked 6 inline comments as done and an inline comment as not 
done.
harlanhaskins added inline comments.



Comment at: clang/lib/ARCMigrate/FileRemapper.cpp:156
+  auto newE = FileMgr->getFile(tempPath);
+  if (newE) {
+remap(origFE, *newE);

jkorous wrote:
> It seems like we're now avoiding the assert in `remap()` we'd hit previously. 
> Is this fine?
If we want to trap if this temp file fails, then I'm happy removing the 
condition check. Do you think this should trap on failure or should it check 
the condition?



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))

jkorous wrote:
> Shouldn't we initialize this guy to `nullptr`?
👍 good catch!



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";

jkorous wrote:
> Could this actually happen? I was expecting the behavior to be consistent 
> with `getFile()`.
Oops, debugging code that needs to be removed. Thanks for catching it!



Comment at: clang/lib/Lex/HeaderSearch.cpp:1448
   if (getHeaderSearchOpts().ModuleMapFileHomeIsCwd)
-Dir = FileMgr.getDirectory(".");
+Dir = *FileMgr.getDirectory(".");
   else {

jkorous wrote:
> Seems like we're introducing assert here. Is that fine?
I'll make this conditional, thanks!



Comment at: clang/unittests/Basic/FileManagerTest.cpp:260
+
+  EXPECT_EQ(f1 ? *f1 : nullptr,
+f2 ? *f2 : nullptr);

jkorous wrote:
> Just an idea - should we compare error codes?
Went ahead and added some tests ensuring we get useful error codes for a couple 
of common issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65534



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Nico Weber via Phabricator via cfe-commits]

thakis added a comment.

Nice! This makes https://bugs.llvm.org/show_bug.cgi?id=42524#c3 easier.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65534



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Raphael Isemann via Phabricator via cfe-commits]

teemperor added a comment.

The error handling in LLDB seems fine to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65534



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Harlan Haskins via Phabricator via cfe-commits]

harlanhaskins updated this revision to Diff 212701.
harlanhaskins added a comment.

Store references instead of raw pointers in FileManger's cache


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65534

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp
  clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
  clang-tools-extra/clang-move/Move.cpp
  clang-tools-extra/clang-move/tool/ClangMove.cpp
  clang-tools-extra/clang-reorder-fields/tool/ClangReorderFields.cpp
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clangd/ClangdUnit.cpp
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/modularize/ModularizeUtilities.cpp
  clang/include/clang/Basic/FileManager.h
  clang/lib/ARCMigrate/FileRemapper.cpp
  clang/lib/ARCMigrate/ObjCMT.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/Basic/FileManager.cpp
  clang/lib/Basic/Module.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Frontend/InitHeaderSearch.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/lib/Frontend/Rewrite/FrontendActions.cpp
  clang/lib/Frontend/TextDiagnostic.cpp
  clang/lib/Lex/HeaderMap.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Lex/PPLexerChange.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/GlobalModuleIndex.cpp
  clang/lib/Serialization/ModuleManager.cpp
  clang/lib/Tooling/Core/Replacement.cpp
  clang/lib/Tooling/Refactoring.cpp
  clang/tools/clang-format/ClangFormat.cpp
  clang/tools/clang-import-test/clang-import-test.cpp
  clang/tools/clang-refactor/ClangRefactor.cpp
  clang/tools/clang-refactor/TestSupport.cpp
  clang/tools/clang-rename/ClangRename.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/Indexing.cpp
  clang/unittests/Basic/FileManagerTest.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp
  clang/unittests/Lex/PPCallbacksTest.cpp
  clang/unittests/Tooling/RefactoringTest.cpp
  clang/unittests/Tooling/RewriterTestContext.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
@@ -247,11 +247,11 @@
 
   bool is_system = true;
   bool is_framework = false;
-  auto *dir =
+  auto dir =
   HS.getFileMgr().getDirectory(module.search_path.GetStringRef());
   if (!dir)
 return error();
-  auto *file = HS.lookupModuleMapFile(dir, is_framework);
+  auto *file = HS.lookupModuleMapFile(*dir, is_framework);
   if (!file)
 return error();
   if (!HS.loadModuleMapFile(file, is_system))
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -908,10 +908,13 @@
   if (file.Write(expr_text, bytes_written).Success()) {
 if (bytes_written == expr_text_len) {
   file.Close();
-  source_mgr.setMainFileID(source_mgr.createFileID(
-  m_compiler->getFileManager().getFile(result_path),
-  SourceLocation(), SrcMgr::C_User));
-  created_main_file = true;
+  if (auto fileEntry =
+  m_compiler->getFileManager().getFile(result_path)) {
+source_mgr.setMainFileID(source_mgr.createFileID(
+*fileEntry,
+SourceLocation(), SrcMgr::C_User));
+created_main_file = true;
+  }
 }
   }
 }
Index: clang/unittests/Tooling/RewriterTestContext.h
===
--- clang/unittests/Tooling/RewriterTestContext.h
+++ clang/unittests/Tooling/RewriterTestContext.h
@@ -56,9 +56,9 @@
 llvm::MemoryBuffer::getMemBuffer(Content);
 InMemoryFileSystem->addFile(Name, 0, std::move(Source));
 
-const FileEntry *Entry = Files.getFile(Name);
-assert(Entry != nullptr);
-return Sources.createFileID(Entry, SourceLocation(), SrcMgr::C_User);
+auto Entry = Files.getFile(Name);
+assert(Entry);
+return Sources.createFileID(*Entry, SourceLocation(), SrcMgr::C_User);
   }
 
   // FIXME: this code is mostly a duplicate of
@@ -73,14 +73,14 @@
 llvm::raw_fd_o

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

[Harlan Haskins via Phabricator via cfe-commits]

harlanhaskins added inline comments.



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.
   ///

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65534



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Jonas Devlieghere via Phabricator via cfe-commits]

JDevlieghere added inline comments.



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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65534



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Harlan Haskins via Phabricator via cfe-commits]

harlanhaskins marked 2 inline comments as done.
harlanhaskins added inline comments.



Comment at: clang/include/clang/Basic/FileManager.h:143
   ///
-  llvm::StringMap SeenDirEntries;
+  llvm::StringMap, llvm::BumpPtrAllocator>
+  SeenDirEntries;

jkorous wrote:
> Maybe we could replace this with some type that has hard-to-use-incorrectly 
> interface instead of using `containsValue()`.
You're right, these should store `llvm::ErrorOr` and use 
`std::err::no_such_file_or_directory` as a placeholder instead of `nullptr`.



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.
   ///

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. 👍


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65534



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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/include/clang/Basic/FileManager.h:143
   ///
-  llvm::StringMap SeenDirEntries;
+  llvm::StringMap, llvm::BumpPtrAllocator>
+  SeenDirEntries;

Maybe we could replace this with some type that has hard-to-use-incorrectly 
interface instead of using `containsValue()`.



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.
   ///

Does that mean that it's now safe to assume the value is `!= NULL` in the 
absence of errors?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65534



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

davide added a comment.

[and Raphael for the clang vendor bits]


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65534



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Davide Italiano via Phabricator via cfe-commits]

davide added a comment.

Really on the lldb side, Jonas is the right person to review this patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65534



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Harlan Haskins via Phabricator via cfe-commits]

harlanhaskins created this revision.
harlanhaskins added reviewers: arphaman, bruno.
Herald added subscribers: lldb-commits, cfe-commits, jsji, kadircet, 
dexonsmith, jkorous, MaskRay, kbarton, nemanjai.
Herald added a reviewer: martong.
Herald added a reviewer: shafik.
Herald added projects: clang, LLDB.
harlanhaskins edited the summary of this revision.
Herald added subscribers: wuzish, ormris, rnkovacs.

Currently, clang's FileManager uses NULL as an indicator that a particular file
did not exist, but would not propagate errors like permission issues. Instead,
teach FileManager to use llvm::ErrorOr internally and return rich errors for
failures.

This patch updates the existing API and updates the callers throughout clang.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D65534

Files:
  clang-tools-extra/clang-apply-replacements/lib/Tooling/ApplyReplacements.cpp
  clang-tools-extra/clang-change-namespace/tool/ClangChangeNamespace.cpp
  clang-tools-extra/clang-include-fixer/IncludeFixer.cpp
  clang-tools-extra/clang-move/Move.cpp
  clang-tools-extra/clang-move/tool/ClangMove.cpp
  clang-tools-extra/clang-reorder-fields/tool/ClangReorderFields.cpp
  clang-tools-extra/clang-tidy/ClangTidy.cpp
  clang-tools-extra/clangd/ClangdUnit.cpp
  clang-tools-extra/clangd/SourceCode.cpp
  clang-tools-extra/clangd/index/SymbolCollector.cpp
  clang-tools-extra/modularize/ModularizeUtilities.cpp
  clang/include/clang/Basic/FileManager.h
  clang/lib/ARCMigrate/FileRemapper.cpp
  clang/lib/ARCMigrate/ObjCMT.cpp
  clang/lib/AST/ASTImporter.cpp
  clang/lib/Basic/FileManager.cpp
  clang/lib/Basic/Module.cpp
  clang/lib/Basic/SourceManager.cpp
  clang/lib/CodeGen/CodeGenAction.cpp
  clang/lib/Frontend/ASTUnit.cpp
  clang/lib/Frontend/CompilerInstance.cpp
  clang/lib/Frontend/FrontendAction.cpp
  clang/lib/Frontend/InitHeaderSearch.cpp
  clang/lib/Frontend/PrecompiledPreamble.cpp
  clang/lib/Frontend/Rewrite/FrontendActions.cpp
  clang/lib/Frontend/TextDiagnostic.cpp
  clang/lib/Lex/HeaderMap.cpp
  clang/lib/Lex/HeaderSearch.cpp
  clang/lib/Lex/ModuleMap.cpp
  clang/lib/Lex/PPDirectives.cpp
  clang/lib/Lex/PPLexerChange.cpp
  clang/lib/Serialization/ASTReader.cpp
  clang/lib/Serialization/GlobalModuleIndex.cpp
  clang/lib/Serialization/ModuleManager.cpp
  clang/lib/Tooling/Core/Replacement.cpp
  clang/lib/Tooling/Refactoring.cpp
  clang/tools/clang-format/ClangFormat.cpp
  clang/tools/clang-import-test/clang-import-test.cpp
  clang/tools/clang-refactor/ClangRefactor.cpp
  clang/tools/clang-refactor/TestSupport.cpp
  clang/tools/clang-rename/ClangRename.cpp
  clang/tools/libclang/CIndex.cpp
  clang/tools/libclang/Indexing.cpp
  clang/unittests/Basic/FileManagerTest.cpp
  clang/unittests/Lex/HeaderSearchTest.cpp
  clang/unittests/Lex/PPCallbacksTest.cpp
  clang/unittests/Tooling/RefactoringTest.cpp
  clang/unittests/Tooling/RewriterTestContext.h
  lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
  lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp

Index: lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp
@@ -247,11 +247,11 @@
 
   bool is_system = true;
   bool is_framework = false;
-  auto *dir =
+  auto dir =
   HS.getFileMgr().getDirectory(module.search_path.GetStringRef());
   if (!dir)
 return error();
-  auto *file = HS.lookupModuleMapFile(dir, is_framework);
+  auto *file = HS.lookupModuleMapFile(*dir, is_framework);
   if (!file)
 return error();
   if (!HS.loadModuleMapFile(file, is_system))
Index: lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
===
--- lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
+++ lldb/source/Plugins/ExpressionParser/Clang/ClangExpressionParser.cpp
@@ -908,10 +908,13 @@
   if (file.Write(expr_text, bytes_written).Success()) {
 if (bytes_written == expr_text_len) {
   file.Close();
-  source_mgr.setMainFileID(source_mgr.createFileID(
-  m_compiler->getFileManager().getFile(result_path),
-  SourceLocation(), SrcMgr::C_User));
-  created_main_file = true;
+  if (auto fileEntry =
+  m_compiler->getFileManager().getFile(result_path)) {
+source_mgr.setMainFileID(source_mgr.createFileID(
+*fileEntry,
+SourceLocation(), SrcMgr::C_User));
+created_main_file = true;
+  }
 }
   }
 }
Index: clang/unittests/Tooling/RewriterTestContext.h
===
--- clang/unittests/Tooling/RewriterTestContext.h
+++ clang/unittests/Tooling/RewriterTestContext