This is an archive of the discontinued LLVM Phabricator instance.

delete SWIG typemaps for FILE*
ClosedPublic

Authored by lawrence_danna on Oct 14 2019, 4:41 PM.

Details

Diff Detail

Event Timeline

lawrence_danna created this revision.Oct 14 2019, 4:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 14 2019, 4:41 PM

🍾

lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
655

You'll need to handle the error here. (And it would be better to replace auto with Expected<PythonFile> as that would make it obvious that the error needs to be handled.)

lawrence_danna marked an inline comment as done.

put in missing consumeError()

lawrence_danna marked an inline comment as done.Oct 15 2019, 10:57 AM
lawrence_danna added inline comments.
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
655

thanks, I don't know why I can't remember to do that.

labath accepted this revision.Oct 16 2019, 4:45 AM
labath added inline comments.
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp
655

Yeah, I don't blame you. The Expected<T> behavior is unlike anything else I've seen, and I myself am still not convinced that such a draconian way of enforcing error checks is in order. However, it is here, and I definitely think it's better than passing around "Status" and T objects separately, like old lldb APIs do.

What you could do is help yourself and reviewers notice that and tune down the "auto" dial (that's also an llvm policy) and spell out the Expected type. Otherwise the type can be confused with Optional<T> (I wish we could say Expected<auto>), which does *not* require checks...

This revision is now accepted and ready to land.Oct 16 2019, 4:45 AM
This revision was automatically updated to reflect the committed changes.
lawrence_danna marked an inline comment as done.