[Lldb-commits] [PATCH] D68995: clean up the implementation of PythonCallable::GetNumArguments

[Lawrence D'Anna via Phabricator via lldb-commits]

lawrence_danna added inline comments.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:58-61
+  auto utf8 = str.AsUTF8();
+  if (!utf8)
+    return utf8.takeError();
+  return utf8.get();
----------------
labath wrote:
> Btw, what are the reasons that this can fail? The string object was created two lines above, so we definitely know it's a string. If the call cannot fail in this particular circumstance, then a one can express that (and make the code simpler) with `return cantFail(str.AsUTF8())`.
It is possible!

```
chr(0xDFFF).encode('utf8')
```


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp:864-891
+    auto function = As<PythonCallable>(globals.GetItem("get_arg_info"));
+    if (!function)
+      return function.takeError();
+    get_arg_info = std::move(function.get());
+  }
+
+  Expected<PythonObject> pyarginfo = get_arg_info.Call(*this);
----------------
labath wrote:
> Likewise, should most of these `takeErrors` be `cantFail` too ?
Yea, i guess given that we control the script there's really no reasonable way these can fail if the first check doesn't.


================
Comment at: lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp:643-647
+    auto arginfo = lambda.GetArgInfo();
+    ASSERT_THAT_EXPECTED(arginfo, llvm::Succeeded());
+    EXPECT_EQ(arginfo.get().count, 1);
+    EXPECT_EQ(arginfo.get().has_varargs, false);
+    EXPECT_EQ(arginfo.get().is_bound_method, false);
----------------
labath wrote:
> If you implemented `operator==` and `operator<<(raw_ostream&)` for the ArgInfo class, you could write this as `EXPECT_THAT_EXPECTED(lambda.GetArgInfo(), llvm::HasValue(ArgInfo(1, false, false))`
eh, do i have to?    I actually like having them each on their own line.   It makes the patches nicer when members are added or removed, and i'd like to remove all the members except max_positional_args.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D68995/new/

https://reviews.llvm.org/D68995