labath added a comment.
I agree about the separate patch stuff, but it seems to be that this should be
done before this one. After all, all (most?) of the existing code has already
been DataObject-ized and this patch is the thing that's deviating from that
practice. I don't think you should rewrite all of the existing code --
hopefully we can find a way to fit this in there. For instance, the CallMethod
thingy would be a new api, so we can have that return expected independently of
the other APIs. I guess the main part you will need is (and that already
existing in the non-Expected form) is the type conversions, but maybe there we
can introduce a new kind of conversion, which is more strict in checking for
errors. Or possibly have the current form return Expected<T>, but then have a
helper function to suppress those errors and return a default-constructed T()
for legacy uses (so legacy code would be sth like
`suppressError(object.AsType<PythonInteger>())`)
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:1369
+ const char *utf8 = PyUnicode_AsUTF8AndSize(pystring.get(), &size);
+ if (!utf8 || PyErr_Occurred())
+ return Status(llvm::make_error<PythonException>("Read"));
----------------
lawrence_danna wrote:
> labath wrote:
> > This is an example where _I think_ you may be using `PyErr_Occurred`
> > incorrectly. If python really asserts that PyErr_Occurred is always called,
> > then this is not right, as you will not call it if the result is null.
> > There are a couple of other examples like that in the code too.
> I think it is correct.
>
> Python doesn't assert that PyErr_Occured is called, it asserts that the error
> is cleared before you call back in to another thing that can cause an error.
> PythonException will clear the error if there is one, or say "unknown
> error" if there isn't one (which should never happen if utf8==NULL, but i'm
> checking out of abundance of caution)
>
>
Ok, thanks for the explanation.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68188/new/
https://reviews.llvm.org/D68188
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
