[Lldb-commits] [PATCH] D69214: remove multi-argument form of PythonObject::Reset()

[Pavel Labath via Phabricator via lldb-commits]

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