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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
