labath added a comment.
I'm at the airport, so I haven't had a chance to look over the full set of
changes you made, but here's my responses to your comments.
================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:778
+ const char *script;
+ const char *function_name;
+ PythonCallable function;
----------------
lawrence_danna wrote:
> labath wrote:
> > I'm wondering if there's a way to avoid passing in the function name
> > argument. Perhaps if the callable is returned as a result of the script
> > ("def foo: ...; return foo")? WDYT?
> I can't use return, but i can have the scripts put their function in a
> well-known output variable.
Yea, I like that. Maybe instead of assigning the defined function to a
well-known some variable, we could just use that name in the `def` declaration
already? Also, aren't names starting with a single underscore considered
"private" in python? Maybe it could just be "function" (or "main", or
"script_main", ...)
================
Comment at:
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h:789-790
+ llvm::Expected<PythonObject> operator()(Args &&... args) {
+ llvm::Error e = Init();
+ if (e)
+ return std::move(e);
----------------
lawrence_danna wrote:
> labath wrote:
> > if (llvm::Error e = ...)
> Is that style considered normative?
>
> I have a strong preference against putting function calls that are being run
> for their side effects inside an `if`.
I didn't find any mention of that in the official style guide, but i think
there's a lot of informal consensus about that. I've seen a lot of reviewers
asking for that style, and I've never seen anyone be asked to change it back.
The main advantage of that is that it reduces the scope of the Error object,
which is particularly important given that Error objects tend to do funny
things when being destroyed.
================
Comment at:
lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp:789-802
+ Expected<PythonObject> r = foo();
+
+ bool failed = !r;
+ ASSERT_TRUE(failed);
+
+ std::string backtrace;
+ llvm::handleAllErrors(r.takeError(), [&](const PythonException &E) {
----------------
lawrence_danna wrote:
> labath wrote:
> > Maybe:
> > ```
> > EXPECT_THAT_EXPECTED(foo(),
> >
> > llvm::Failed<PythonException>(testing::Property(&PythonException::ReadBacktrace,
> > testing::ContainsRegex(...) )) ));
> > ```
> that doesn't seem to support multiple regexes, and is doing it 5 times really
> any more clear than just doing it manually?
I was thinking you would just do a `ContainsRegex("line 3.*line 5.*line 7...")`
If you want to emulate the current behavior precisely, you could do a
`MatchesAll(HasSubstr("line 3"), HasSubstr("line 5"), ...)`.
The main advantage I see here is in the error message that you get when this
thing fails. In the current setup, you'll get " expected 0xffff != 0xffff, got
0xffff == 0xffff". This expression will end up producing the full error string
as the "obtained" value, which will hopefully give you some clue about what
went wrong. Which is particuarly useful if the failure happens only on some
bot, whose configuration you can't easily reproduce.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69214/new/
https://reviews.llvm.org/D69214
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
