[Lldb-commits] [PATCH] D67996: Convert FileSystem::Open() to return Expected<FileUP>

[Pavel Labath via Phabricator via lldb-commits]

labath accepted this revision.
labath added a comment.
This revision is now accepted and ready to land.

looks good to me, a extra couple of  small comments inline. Thank you for taking your time to do this.



================
Comment at: lldb/include/lldb/Core/StreamFile.h:38
 
+  StreamFile(std::shared_ptr<File> file) : m_file_sp(file){};
+
----------------
assert `file` is not null?


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1034
 
-bool PythonFile::GetUnderlyingFile(File &file) const {
+bool PythonFile::GetUnderlyingFile(FileSP &file) const {
   if (!IsValid())
----------------
This only has one caller, so it should be pretty simple to make it return the file as an actual return value (and it looks like it could be returning a unique_ptr).


================
Comment at: lldb/unittests/Host/FileSystemTest.cpp:292-322
+TEST(FileSystemTest, OpenErrno) {
+#ifdef _WIN32
+  FileSpec spec("C:\\FILE\\THAT\\DOES\\NOT\\EXIST.TXT");
+#else
+  FileSpec spec("/file/that/does/not/exist.txt");
+#endif
+  FileSystem fs;
----------------
lawrence_danna wrote:
> labath wrote:
> > What's the point of having both of these tests? The error code should be the same no matter how you retrieve it from the expected object, so this is more of a unit test for the Expected class, than anything else...
> GDBRemoteCommunicationServerCommon.cpp collects and errno value and sends it over the network.   I wanted to confirm FileSystem::Open() was returning and Error value that could be correctly converted into errno.
Yes, I got that part (and it's great that you've added it). But why test the same thing two ways? My point was that these two tests should always either both succeed, or both fail (any other result would be a bug in the Expected class).

BTW, if you wanted to be really fancy, you could write this as something like
```
EXPECT_THAT_EXPECTED(fs.Open(spec, File::eOpenOptionRead, 0, true), 
  llvm::Failed<ECError>(testing::Property(&ECError::convertToErrorCode, 
                                         std::error_code(ENOENT, std::system_category())));
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67996