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

2019-08-01 Thread 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=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

2019-08-01 Thread 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

2019-08-01 Thread 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

2019-07-31 Thread 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 @@

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

2019-07-31 Thread 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

2019-07-31 Thread 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

2019-07-31 Thread 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
+++

[PATCH] D60786: [FileSystemStatCache] Update test for new FileSystemStatCache API

2019-04-16 Thread Harlan Haskins via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rC358511: [FileSystemStatCache] Update test for new 
FileSystemStatCache API (authored by harlanhaskins, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D60786?vs=195416=195417#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D60786

Files:
  unittests/Basic/FileManagerTest.cpp


Index: unittests/Basic/FileManagerTest.cpp
===
--- unittests/Basic/FileManagerTest.cpp
+++ unittests/Basic/FileManagerTest.cpp
@@ -57,9 +57,10 @@
   }
 
   // Implement FileSystemStatCache::getStat().
-  LookupResult getStat(StringRef Path, llvm::vfs::Status , bool isFile,
-   std::unique_ptr *F,
-   llvm::vfs::FileSystem ) override {
+  std::error_code getStat(StringRef Path, llvm::vfs::Status ,
+  bool isFile,
+  std::unique_ptr *F,
+  llvm::vfs::FileSystem ) override {
 #ifndef _WIN32
 SmallString<128> NormalizedPath(Path);
 llvm::sys::path::native(NormalizedPath);
@@ -68,10 +69,10 @@
 
 if (StatCalls.count(Path) != 0) {
   Status = StatCalls[Path];
-  return CacheExists;
+  return std::error_code();
 }
 
-return CacheMissing;  // This means the file/directory doesn't exist.
+return std::make_error_code(std::errc::no_such_file_or_directory);
   }
 };
 


Index: unittests/Basic/FileManagerTest.cpp
===
--- unittests/Basic/FileManagerTest.cpp
+++ unittests/Basic/FileManagerTest.cpp
@@ -57,9 +57,10 @@
   }
 
   // Implement FileSystemStatCache::getStat().
-  LookupResult getStat(StringRef Path, llvm::vfs::Status , bool isFile,
-   std::unique_ptr *F,
-   llvm::vfs::FileSystem ) override {
+  std::error_code getStat(StringRef Path, llvm::vfs::Status ,
+  bool isFile,
+  std::unique_ptr *F,
+  llvm::vfs::FileSystem ) override {
 #ifndef _WIN32
 SmallString<128> NormalizedPath(Path);
 llvm::sys::path::native(NormalizedPath);
@@ -68,10 +69,10 @@
 
 if (StatCalls.count(Path) != 0) {
   Status = StatCalls[Path];
-  return CacheExists;
+  return std::error_code();
 }
 
-return CacheMissing;  // This means the file/directory doesn't exist.
+return std::make_error_code(std::errc::no_such_file_or_directory);
   }
 };
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60786: [FileSystemStatCache] Update test for new FileSystemStatCache API

2019-04-16 Thread Harlan Haskins via Phabricator via cfe-commits 
harlanhaskins created this revision.
harlanhaskins added a reviewer: arphaman.
Herald added subscribers: cfe-commits, dexonsmith.
Herald added a project: clang.
arphaman accepted this revision.
arphaman added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: ormris.

LG


Update this test to return std::error_code instead of LookupResult.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60786

Files:
  clang/unittests/Basic/FileManagerTest.cpp


Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -57,9 +57,10 @@
   }
 
   // Implement FileSystemStatCache::getStat().
-  LookupResult getStat(StringRef Path, llvm::vfs::Status , bool isFile,
-   std::unique_ptr *F,
-   llvm::vfs::FileSystem ) override {
+  std::error_code getStat(StringRef Path, llvm::vfs::Status ,
+  bool isFile,
+  std::unique_ptr *F,
+  llvm::vfs::FileSystem ) override {
 #ifndef _WIN32
 SmallString<128> NormalizedPath(Path);
 llvm::sys::path::native(NormalizedPath);
@@ -68,10 +69,10 @@
 
 if (StatCalls.count(Path) != 0) {
   Status = StatCalls[Path];
-  return CacheExists;
+  return std::error_code();
 }
 
-return CacheMissing;  // This means the file/directory doesn't exist.
+return std::make_error_code(std::errc::no_such_file_or_directory);
   }
 };
 


Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -57,9 +57,10 @@
   }
 
   // Implement FileSystemStatCache::getStat().
-  LookupResult getStat(StringRef Path, llvm::vfs::Status , bool isFile,
-   std::unique_ptr *F,
-   llvm::vfs::FileSystem ) override {
+  std::error_code getStat(StringRef Path, llvm::vfs::Status ,
+  bool isFile,
+  std::unique_ptr *F,
+  llvm::vfs::FileSystem ) override {
 #ifndef _WIN32
 SmallString<128> NormalizedPath(Path);
 llvm::sys::path::native(NormalizedPath);
@@ -68,10 +69,10 @@
 
 if (StatCalls.count(Path) != 0) {
   Status = StatCalls[Path];
-  return CacheExists;
+  return std::error_code();
 }
 
-return CacheMissing;  // This means the file/directory doesn't exist.
+return std::make_error_code(std::errc::no_such_file_or_directory);
   }
 };
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60735: [FileSystemStatCache] Return std::error_code from stat cache methods

2019-04-16 Thread Harlan Haskins via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rC358509: [FileSystemStatCache] Return std::error_code from 
stat cache methods (authored by harlanhaskins, committed by ).

Changed prior to commit:
  https://reviews.llvm.org/D60735?vs=195264=195411#toc

Repository:
  rC Clang

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

https://reviews.llvm.org/D60735

Files:
  include/clang/Basic/FileSystemStatCache.h
  lib/Basic/FileManager.cpp
  lib/Basic/FileSystemStatCache.cpp

Index: include/clang/Basic/FileSystemStatCache.h
===
--- include/clang/Basic/FileSystemStatCache.h
+++ include/clang/Basic/FileSystemStatCache.h
@@ -37,14 +37,6 @@
 public:
   virtual ~FileSystemStatCache() = default;
 
-  enum LookupResult {
-/// We know the file exists and its cached stat data.
-CacheExists,
-
-/// We know that the file doesn't exist.
-CacheMissing
-  };
-
   /// Get the 'stat' information for the specified path, using the cache
   /// to accelerate it if possible.
   ///
@@ -55,18 +47,19 @@
   /// success for directories (not files).  On a successful file lookup, the
   /// implementation can optionally fill in \p F with a valid \p File object and
   /// the client guarantees that it will close it.
-  static bool get(StringRef Path, llvm::vfs::Status , bool isFile,
-  std::unique_ptr *F,
-  FileSystemStatCache *Cache, llvm::vfs::FileSystem );
+  static std::error_code
+  get(StringRef Path, llvm::vfs::Status , bool isFile,
+  std::unique_ptr *F,
+  FileSystemStatCache *Cache, llvm::vfs::FileSystem );
 
 protected:
   // FIXME: The pointer here is a non-owning/optional reference to the
   // unique_ptr. Optional&> might be nicer, but
   // Optional needs some work to support references so this isn't possible yet.
-  virtual LookupResult getStat(StringRef Path, llvm::vfs::Status ,
-   bool isFile,
-   std::unique_ptr *F,
-   llvm::vfs::FileSystem ) = 0;
+  virtual std::error_code getStat(StringRef Path, llvm::vfs::Status ,
+  bool isFile,
+  std::unique_ptr *F,
+  llvm::vfs::FileSystem ) = 0;
 };
 
 /// A stat "cache" that can be used by FileManager to keep
@@ -84,9 +77,10 @@
   iterator begin() const { return StatCalls.begin(); }
   iterator end() const { return StatCalls.end(); }
 
-  LookupResult getStat(StringRef Path, llvm::vfs::Status , bool isFile,
-   std::unique_ptr *F,
-   llvm::vfs::FileSystem ) override;
+  std::error_code getStat(StringRef Path, llvm::vfs::Status ,
+  bool isFile,
+  std::unique_ptr *F,
+  llvm::vfs::FileSystem ) override;
 };
 
 } // namespace clang
Index: lib/Basic/FileManager.cpp
===
--- lib/Basic/FileManager.cpp
+++ lib/Basic/FileManager.cpp
@@ -429,13 +429,14 @@
   // FIXME: FileSystemOpts shouldn't be passed in here, all paths should be
   // absolute!
   if (FileSystemOpts.WorkingDir.empty())
-return FileSystemStatCache::get(Path, Status, isFile, F,StatCache.get(), *FS);
+return bool(FileSystemStatCache::get(Path, Status, isFile, F,
+ StatCache.get(), *FS));
 
   SmallString<128> FilePath(Path);
   FixupRelativePath(FilePath);
 
-  return FileSystemStatCache::get(FilePath.c_str(), Status, isFile, F,
-  StatCache.get(), *FS);
+  return bool(FileSystemStatCache::get(FilePath.c_str(), Status, isFile, F,
+   StatCache.get(), *FS));
 }
 
 bool FileManager::getNoncachedStatValue(StringRef Path,
Index: lib/Basic/FileSystemStatCache.cpp
===
--- lib/Basic/FileSystemStatCache.cpp
+++ lib/Basic/FileSystemStatCache.cpp
@@ -30,25 +30,24 @@
 /// success for directories (not files).  On a successful file lookup, the
 /// implementation can optionally fill in FileDescriptor with a valid
 /// descriptor and the client guarantees that it will close it.
-bool FileSystemStatCache::get(StringRef Path, llvm::vfs::Status ,
-  bool isFile,
-  std::unique_ptr *F,
-  FileSystemStatCache *Cache,
-  llvm::vfs::FileSystem ) {
-  LookupResult R;
+std::error_code
+FileSystemStatCache::get(StringRef Path, llvm::vfs::Status ,
+ bool isFile, std::unique_ptr *F,
+ FileSystemStatCache *Cache,
+ llvm::vfs::FileSystem ) {
   bool isForDir = !isFile;
+  std::error_code RetCode;
 
   // If we have a cache, use it to resolve the stat query.

[PATCH] D60735: [FileSystemStatCache] Return std::error_code from stat cache methods

2019-04-15 Thread Harlan Haskins via Phabricator via cfe-commits 
harlanhaskins created this revision.
harlanhaskins added a reviewer: benlangmuir.
Herald added subscribers: cfe-commits, dexonsmith.
Herald added a project: clang.

Previously, we would return true/false signifying if the cache/lookup
succeeded or failed. Instead, provide clients with the underlying error
that was thrown while attempting to look up in the cache.

Since clang::FileManager doesn't make use of this information, it discards the
error that's received and casts away to bool.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60735

Files:
  clang/include/clang/Basic/FileSystemStatCache.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/Basic/FileSystemStatCache.cpp

Index: clang/lib/Basic/FileSystemStatCache.cpp
===
--- clang/lib/Basic/FileSystemStatCache.cpp
+++ clang/lib/Basic/FileSystemStatCache.cpp
@@ -30,25 +30,24 @@
 /// success for directories (not files).  On a successful file lookup, the
 /// implementation can optionally fill in FileDescriptor with a valid
 /// descriptor and the client guarantees that it will close it.
-bool FileSystemStatCache::get(StringRef Path, llvm::vfs::Status ,
-  bool isFile,
-  std::unique_ptr *F,
-  FileSystemStatCache *Cache,
-  llvm::vfs::FileSystem ) {
-  LookupResult R;
+std::error_code
+FileSystemStatCache::get(StringRef Path, llvm::vfs::Status ,
+ bool isFile, std::unique_ptr *F,
+ FileSystemStatCache *Cache,
+ llvm::vfs::FileSystem ) {
   bool isForDir = !isFile;
+  std::error_code RetCode;
 
   // If we have a cache, use it to resolve the stat query.
   if (Cache)
-R = Cache->getStat(Path, Status, isFile, F, FS);
+RetCode = Cache->getStat(Path, Status, isFile, F, FS);
   else if (isForDir || !F) {
 // If this is a directory or a file descriptor is not needed and we have
 // no cache, just go to the file system.
 llvm::ErrorOr StatusOrErr = FS.status(Path);
 if (!StatusOrErr) {
-  R = CacheMissing;
+  RetCode = StatusOrErr.getError();
 } else {
-  R = CacheExists;
   Status = *StatusOrErr;
 }
   } else {
@@ -63,27 +62,27 @@
 
 if (!OwnedFile) {
   // If the open fails, our "stat" fails.
-  R = CacheMissing;
+  RetCode = OwnedFile.getError();
 } else {
   // Otherwise, the open succeeded.  Do an fstat to get the information
   // about the file.  We'll end up returning the open file descriptor to the
   // client to do what they please with it.
   llvm::ErrorOr StatusOrErr = (*OwnedFile)->status();
   if (StatusOrErr) {
-R = CacheExists;
 Status = *StatusOrErr;
 *F = std::move(*OwnedFile);
   } else {
 // fstat rarely fails.  If it does, claim the initial open didn't
 // succeed.
-R = CacheMissing;
 *F = nullptr;
+RetCode = StatusOrErr.getError();
   }
 }
   }
 
   // If the path doesn't exist, return failure.
-  if (R == CacheMissing) return true;
+  if (RetCode)
+return RetCode;
 
   // If the path exists, make sure that its "directoryness" matches the clients
   // demands.
@@ -91,29 +90,31 @@
 // If not, close the file if opened.
 if (F)
   *F = nullptr;
-
-return true;
+return std::make_error_code(
+Status.isDirectory() ?
+std::errc::is_a_directory : std::errc::not_a_directory);
   }
 
-  return false;
+  return std::error_code();
 }
 
-MemorizeStatCalls::LookupResult
+std::error_code
 MemorizeStatCalls::getStat(StringRef Path, llvm::vfs::Status ,
bool isFile,
std::unique_ptr *F,
llvm::vfs::FileSystem ) {
-  if (get(Path, Status, isFile, F, nullptr, FS)) {
+  auto err = get(Path, Status, isFile, F, nullptr, FS);
+  if (err) {
 // Do not cache failed stats, it is easy to construct common inconsistent
 // situations if we do, and they are not important for PCH performance
 // (which currently only needs the stats to construct the initial
 // FileManager entries).
-return CacheMissing;
+return err;
   }
 
   // Cache file 'stat' results and directories with absolutely paths.
   if (!Status.isDirectory() || llvm::sys::path::is_absolute(Path))
 StatCalls[Path] = Status;
 
-  return CacheExists;
+  return std::error_code();
 }
Index: clang/lib/Basic/FileManager.cpp
===
--- clang/lib/Basic/FileManager.cpp
+++ clang/lib/Basic/FileManager.cpp
@@ -429,13 +429,14 @@
   // FIXME: FileSystemOpts shouldn't be passed in here, all paths should be
   // absolute!
   if (FileSystemOpts.WorkingDir.empty())
-return FileSystemStatCache::get(Path, Status, isFile, F,StatCache.get(), *FS);
+return

[PATCH] D58924: Replace clang::FileData with llvm::vfs::Status

2019-03-04 Thread Harlan Haskins via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL355368: Replace clang::FileData with llvm::vfs::Status 
(authored by harlanhaskins, committed by ).
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

Changed prior to commit:
  https://reviews.llvm.org/D58924?vs=189202=189258#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D58924

Files:
  cfe/trunk/include/clang/Basic/FileManager.h
  cfe/trunk/include/clang/Basic/FileSystemStatCache.h
  cfe/trunk/lib/Basic/FileManager.cpp
  cfe/trunk/lib/Basic/FileSystemStatCache.cpp
  cfe/trunk/lib/Frontend/TextDiagnostic.cpp
  cfe/trunk/unittests/Basic/FileManagerTest.cpp

Index: cfe/trunk/lib/Basic/FileManager.cpp
===
--- cfe/trunk/lib/Basic/FileManager.cpp
+++ cfe/trunk/lib/Basic/FileManager.cpp
@@ -144,8 +144,8 @@
   StringRef InterndDirName = NamedDirEnt.first();
 
   // Check to see if the directory exists.
-  FileData Data;
-  if (getStatValue(InterndDirName, Data, false, nullptr /*directory lookup*/)) {
+  llvm::vfs::Status Status;
+  if (getStatValue(InterndDirName, Status, false, nullptr /*directory lookup*/)) {
 // There's no real directory at the given path.
 if (!CacheFailure)
   SeenDirEntries.erase(DirName);
@@ -156,7 +156,7 @@
   // same inode (this occurs on Unix-like systems when one dir is
   // symlinked to another, for example) or the same path (on
   // Windows).
-  DirectoryEntry  = UniqueRealDirs[Data.UniqueID];
+  DirectoryEntry  = UniqueRealDirs[Status.getUniqueID()];
 
   NamedDirEnt.second = 
   if (UDE.getName().empty()) {
@@ -205,8 +205,8 @@
 
   // Check to see if the file exists.
   std::unique_ptr F;
-  FileData Data;
-  if (getStatValue(InterndFileName, Data, true, openFile ?  : nullptr)) {
+  llvm::vfs::Status Status;
+  if (getStatValue(InterndFileName, Status, true, openFile ?  : nullptr)) {
 // There's no real file at the given path.
 if (!CacheFailure)
   SeenFileEntries.erase(Filename);
@@ -218,14 +218,15 @@
 
   // It exists.  See if we have already opened a file with the same inode.
   // This occurs when one dir is symlinked to another, for example.
-  FileEntry  = UniqueRealFiles[Data.UniqueID];
+  FileEntry  = UniqueRealFiles[Status.getUniqueID()];
 
   NamedFileEnt.second = 
 
   // If the name returned by getStatValue is different than Filename, re-intern
   // the name.
-  if (Data.Name != Filename) {
-auto  = *SeenFileEntries.insert({Data.Name, }).first;
+  if (Status.getName() != Filename) {
+auto  =
+  *SeenFileEntries.insert({Status.getName(), }).first;
 assert(NamedFileEnt.second ==  &&
"filename from getStatValue() refers to wrong file");
 InterndFileName = NamedFileEnt.first().data();
@@ -239,7 +240,7 @@
 // module's structure when its headers/module map are mapped in the VFS.
 // We should remove this as soon as we can properly support a file having
 // multiple names.
-if (DirInfo != UFE.Dir && Data.IsVFSMapped)
+if (DirInfo != UFE.Dir && Status.IsVFSMapped)
   UFE.Dir = DirInfo;
 
 // Always update the name to use the last name by which a file was accessed.
@@ -254,13 +255,12 @@
 
   // Otherwise, we don't have this file yet, add it.
   UFE.Name= InterndFileName;
-  UFE.Size = Data.Size;
-  UFE.ModTime = Data.ModTime;
+  UFE.Size= Status.getSize();
+  UFE.ModTime = llvm::sys::toTimeT(Status.getLastModificationTime());
   UFE.Dir = DirInfo;
   UFE.UID = NextFileUID++;
-  UFE.UniqueID = Data.UniqueID;
-  UFE.IsNamedPipe = Data.IsNamedPipe;
-  UFE.InPCH = Data.InPCH;
+  UFE.UniqueID = Status.getUniqueID();
+  UFE.IsNamedPipe = Status.getType() == llvm::sys::fs::file_type::fifo_file;
   UFE.File = std::move(F);
   UFE.IsValid = true;
 
@@ -298,12 +298,15 @@
  "The directory of a virtual file should already be in the cache.");
 
   // Check to see if the file exists. If so, drop the virtual file
-  FileData Data;
+  llvm::vfs::Status Status;
   const char *InterndFileName = NamedFileEnt.first().data();
-  if (getStatValue(InterndFileName, Data, true, nullptr) == 0) {
-Data.Size = Size;
-Data.ModTime = ModificationTime;
-UFE = [Data.UniqueID];
+  if (getStatValue(InterndFileName, Status, true, nullptr) == 0) {
+UFE = [Status.getUniqueID()];
+Status = llvm::vfs::Status(
+  Status.getName(), Status.getUniqueID(),
+  llvm::sys::toTimePoint(ModificationTime),
+  Status.getUser(), Status.getGroup(), Size,
+  Status.getType(), Status.getPermissions());
 
 NamedFileEnt.second = UFE;
 
@@ -317,10 +320,9 @@
 if (UFE->isValid())
   return UFE;
 
-UFE->UniqueID = Data.UniqueID;
-UFE->IsNamedPipe = Data.IsNamedPipe;
-UFE->InPCH = Data.InPCH;
-fillRealPathName(UFE, Data.Name);
+UFE->UniqueID = Status.getUniqueID();
+UFE->IsNamedPipe = Status.getType() ==

[PATCH] D58924: Replace clang::FileData with llvm::vfs::Status

2019-03-04 Thread Harlan Haskins via Phabricator via cfe-commits 
harlanhaskins marked an inline comment as done.
harlanhaskins added inline comments.



Comment at: clang/lib/Basic/FileManager.cpp:305
+UFE = [Status.getUniqueID()];
+Status = llvm::vfs::Status(
+  Status.getName(), Status.getUniqueID(),

benlangmuir wrote:
> Why copy Status back into Status instead of mutating the relevant fields?
The fields don't have setters exposed, and I couldn't decide if adding the 
setters vs. re-constructing a Status was better here. Would it be worth adding 
setters to the properties in `Status`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D58924



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

[PATCH] D58924: Replace clang::FileData with llvm::vfs::Status

2019-03-04 Thread Harlan Haskins via Phabricator via cfe-commits 
harlanhaskins created this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
harlanhaskins edited the summary of this revision.
harlanhaskins added a reviewer: benlangmuir.
Herald added a subscriber: ormris.

`FileData` was only ever used as a container for the values in
`llvm::vfs::Status`, so they might as well be consolidated.

The `InPCH` member was also always set to false, and unused.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D58924

Files:
  clang/include/clang/Basic/FileManager.h
  clang/include/clang/Basic/FileSystemStatCache.h
  clang/lib/Basic/FileManager.cpp
  clang/lib/Basic/FileSystemStatCache.cpp
  clang/lib/Frontend/TextDiagnostic.cpp
  clang/unittests/Basic/FileManagerTest.cpp

Index: clang/unittests/Basic/FileManagerTest.cpp
===
--- clang/unittests/Basic/FileManagerTest.cpp
+++ clang/unittests/Basic/FileManagerTest.cpp
@@ -26,7 +26,7 @@
 private:
   // Maps a file/directory path to its desired stat result.  Anything
   // not in this map is considered to not exist in the file system.
-  llvm::StringMap StatCalls;
+  llvm::StringMap StatCalls;
 
   void InjectFileOrDirectory(const char *Path, ino_t INode, bool IsFile) {
 #ifndef _WIN32
@@ -35,15 +35,14 @@
 Path = NormalizedPath.c_str();
 #endif
 
-FileData Data;
-Data.Name = Path;
-Data.Size = 0;
-Data.ModTime = 0;
-Data.UniqueID = llvm::sys::fs::UniqueID(1, INode);
-Data.IsDirectory = !IsFile;
-Data.IsNamedPipe = false;
-Data.InPCH = false;
-StatCalls[Path] = Data;
+auto fileType = IsFile ?
+  llvm::sys::fs::file_type::regular_file :
+  llvm::sys::fs::file_type::directory_file;
+llvm::vfs::Status Status(Path, llvm::sys::fs::UniqueID(1, INode),
+ /*MTime*/{}, /*User*/0, /*Group*/0,
+ /*Size*/0, fileType,
+ llvm::sys::fs::perms::all_all);
+StatCalls[Path] = Status;
   }
 
 public:
@@ -58,7 +57,7 @@
   }
 
   // Implement FileSystemStatCache::getStat().
-  LookupResult getStat(StringRef Path, FileData , bool isFile,
+  LookupResult getStat(StringRef Path, llvm::vfs::Status , bool isFile,
std::unique_ptr *F,
llvm::vfs::FileSystem ) override {
 #ifndef _WIN32
@@ -68,7 +67,7 @@
 #endif
 
 if (StatCalls.count(Path) != 0) {
-  Data = StatCalls[Path];
+  Status = StatCalls[Path];
   return CacheExists;
 }
 
Index: clang/lib/Frontend/TextDiagnostic.cpp
===
--- clang/lib/Frontend/TextDiagnostic.cpp
+++ clang/lib/Frontend/TextDiagnostic.cpp
@@ -792,8 +792,6 @@
   const FileEntry *FE = Loc.getFileEntry();
   if (FE && FE->isValid()) {
 emitFilename(FE->getName(), Loc.getManager());
-if (FE->isInPCH())
-  OS << " (in PCH)";
 OS << ": ";
   }
 }
Index: clang/lib/Basic/FileSystemStatCache.cpp
===
--- clang/lib/Basic/FileSystemStatCache.cpp
+++ clang/lib/Basic/FileSystemStatCache.cpp
@@ -21,18 +21,6 @@
 
 void FileSystemStatCache::anchor() {}
 
-static void copyStatusToFileData(const llvm::vfs::Status ,
- FileData ) {
-  Data.Name = Status.getName();
-  Data.Size = Status.getSize();
-  Data.ModTime = llvm::sys::toTimeT(Status.getLastModificationTime());
-  Data.UniqueID = Status.getUniqueID();
-  Data.IsDirectory = Status.isDirectory();
-  Data.IsNamedPipe = Status.getType() == llvm::sys::fs::file_type::fifo_file;
-  Data.InPCH = false;
-  Data.IsVFSMapped = Status.IsVFSMapped;
-}
-
 /// FileSystemStatCache::get - Get the 'stat' information for the specified
 /// path, using the cache to accelerate it if possible.  This returns true if
 /// the path does not exist or false if it exists.
@@ -42,7 +30,8 @@
 /// success for directories (not files).  On a successful file lookup, the
 /// implementation can optionally fill in FileDescriptor with a valid
 /// descriptor and the client guarantees that it will close it.
-bool FileSystemStatCache::get(StringRef Path, FileData , bool isFile,
+bool FileSystemStatCache::get(StringRef Path, llvm::vfs::Status ,
+  bool isFile,
   std::unique_ptr *F,
   FileSystemStatCache *Cache,
   llvm::vfs::FileSystem ) {
@@ -51,16 +40,16 @@
 
   // If we have a cache, use it to resolve the stat query.
   if (Cache)
-R = Cache->getStat(Path, Data, isFile, F, FS);
+R = Cache->getStat(Path, Status, isFile, F, FS);
   else if (isForDir || !F) {
 // If this is a directory or a file descriptor is not needed and we have
 // no cache, just go to the file system.
-llvm::ErrorOr Status = FS.status(Path);
-if (!Status) {
+llvm::ErrorOr StatusOrErr = FS.status(Path);
+if