Page MenuHomePhabricator

Convert FileSystem::Open() to return Expected<FileUP>

Authored by lawrence_danna on Sep 24 2019, 4:08 PM.



This patch converts FileSystem::Open from this prototype:

Open(File &File, const FileSpec &file_spec, ...);

to this one:

Open(const FileSpec &file_spec, ...);

This is beneficial on its own, as llvm::Expected is a more modern
and recommended error type than Status. It is also a necessary step
towards, and further developments
for lldb_private::File.

Diff Detail


Event Timeline

lawrence_danna created this revision.Sep 24 2019, 4:08 PM
labath marked an inline comment as done.Sep 25 2019, 5:11 AM

Thanks for splitting this up. Despite the number of comments, I think this is looking pretty good. The main theme of the comments is making sure that we satisfy the contract of the Expected class, which is a bit... unexpected. This is something that every new contributor gets burned by so don't worry about that. I've tried to annotate each place and suggest the possible way to handle it.

Additionally, I think it would be good to remove the ability to swap the file from underneath the stream if it is not too difficult.

48–53 ↗(On Diff #221618)

Could we remove this method? It would make things easier to reason about if we could disallow swapping of a File object backing a stream midway through its lifetime. Looking at the existing callers, it does not seem it should be hard to do that -- this is always called immediately after a stream is constructed via patterns like:

auto *stream = new StreamFile();
auto file = create_a_file();
if (file.is_ok()) {

It should be easy to change that so that the stream is constructed only after we have a valid File object. It would also avoid the need to construct a fake File object just to guarantee it is always initialized.

93 ↗(On Diff #221618)

This is the trickiest thing about the Expected class. The idea behind it is it forces you to do something with the error. This means that this code would trigger an assertion if the opening of the file ever fails, as you don't "handle" the error in any way.
Unfortunately, here there is nothing we can do with the error (this is probably a deficiency of this API, but that's also another thing we can't change here), so we need to either explicitly throw it away (else consumeError(file.takeError())), or log it else LLDB_LOG_ERROR(GetLogIfAllCategoriesSet(LIBLLDB_LOG_API), file.takeError(), "Cannot open {1}: {0}", path)

120 ↗(On Diff #221618)

why not assign this to the m_opaque_up (and avoid having an unowned pointer floating around)?

143 ↗(On Diff #221618)

same here.

807–808 ↗(On Diff #221618)

Include the error message here, which will also set the "checked" flag inside the expected object.

40 ↗(On Diff #221618)

We'll need to ignore/log the error here. Maybe leave a TODO to refactor this so that the caller is able to know whether the file was successfully created or not.

49 ↗(On Diff #221618)

same here.

412 ↗(On Diff #221618)

I guess the error can be written to error_sp here.

33 ↗(On Diff #221618)

add: error = std::move(file.takeError());

40 ↗(On Diff #221618)

return errorToBool(imageBinaryUP.takeError())

2326–2329 ↗(On Diff #221618)

You could change this into something like result.AppendErrorWithFormatv("error: an error occurred read file '{0}': {1}\n", cmd_file_path, fmt_consume(file.takeError()))

2659 ↗(On Diff #221618)

add the actual error to the error message, setting the checked flag

4595 ↗(On Diff #221618)

add the actual error here

6279 ↗(On Diff #221618)

this is good :)

107 ↗(On Diff #221618)

ignore the error, I guess?

958 ↗(On Diff #221618)


913 ↗(On Diff #221618)

add the error to the error message (you'll probably need to check each variable separately)

1232 ↗(On Diff #221618)

return Status(source_file.takeError())

292–322 ↗(On Diff #221618)

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

586 ↗(On Diff #221618)

ASSERT_THAT_EXPECTED(file, llvm::Succeeded())

lawrence_danna marked 19 inline comments as done.Sep 25 2019, 2:58 PM

wow, I didn't realize the part about actually consuming the error. I thought the asserts only checked that you checked if there was an error. Uploading fixes momentarily

292–322 ↗(On Diff #221618)

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.

lawrence_danna marked an inline comment as done.

Fixed according to reviewer comments.

got rid of StreamFile::SetFile

lawrence_danna marked 2 inline comments as done.Sep 25 2019, 4:58 PM
lawrence_danna added inline comments.
48–53 ↗(On Diff #221618)

Yea, it wasn't' too complicated to get rid of it.

lawrence_danna marked an inline comment as done.Sep 25 2019, 5:02 PM
labath accepted this revision.Sep 26 2019, 5:23 AM

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

38 ↗(On Diff #221864)

assert file is not null?

1034 ↗(On Diff #221864)

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

292–322 ↗(On Diff #221618)

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), 
                                         std::error_code(ENOENT, std::system_category())));
This revision is now accepted and ready to land.Sep 26 2019, 5:23 AM
lawrence_danna marked 3 inline comments as done.

fixed the remaining comments

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptSep 26 2019, 10:54 AM