With this patch, only the no-argument form of Reset() remains in
PythonDataObjects. It also deletes PythonExceptionState in favor of
PythonException, because the only call-site of PythonExceptionState was
also using Reset, so I cleaned up both while I was there.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 39824 Build 39871: arc lint + arc unit
Event Timeline
No fudnamental issues with this patch (I'm ignoring the CStringArg topic here). I do think that it would be nice to have some more unit tests for the new APIs you're introducing (or elevating to API status). I'm mainly thinking of the PythonScript stuff (a test where we evaluate the script successfully) and the runString functions.
lldb/include/lldb/Interpreter/ScriptInterpreter.h | ||
---|---|---|
68–70 | I should've mentioned this earlier, but llvm style is to generally use // comments https://llvm.org/docs/CodingStandards.html#comment-formatting., | |
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp | ||
262–265 | Do I understand correctly that the idea is to eventually replace these functions with constructor calls too? | |
1059 | I just realized I don't actually know what this means. Is it that the object takes the GIL internally? Or that one must have the GIL taken before he starts to interact with it? Maybe it'd be better to clarify this in the class description and just delete these comments. | |
1066–1067 | maybe just return toString(backtrace.takeError()) here? | |
1530 | Use early return here (if (valid) return Success;) | |
1534–1535 | if (Error error = ...) | |
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h | ||
327 | Maybe it's time to move the entire PythonObject hierarchy into the python namespace? That would be consistent with some of the newer lldb plugins, which have all of their code inside a sub-namespace of lldb_private.. | |
755 | 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? | |
766–767 | if (llvm::Error e = ...) | |
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPython.cpp | ||
1172–1183 | Would something like this also work? llvm::Error error = handleErrors(return_value.takeError(), [](PythonException &E) { llvm::Error str = makeStringError(E.ReadBacktrace()); if (!options.GetMaskoutErrors) E.Restore(); return str; }); return std::move(error); | |
1982–1983 | If doesn't look like this Reset call is really needed here.. | |
lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp | ||
790–803 | Maybe: EXPECT_THAT_EXPECTED(foo(), llvm::Failed<PythonException>(testing::Property(&PythonException::ReadBacktrace, testing::ContainsRegex(...) )) )); |
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp | ||
---|---|---|
1059 | Almost everything in PythonDataObjects requires you to call it with the GIL already locked. The only things that don't are the File methods because they're called by things that don't know anything about python. Normally when I need to do something like this I would use dispatch_once, or whatever the LLVM version of that is, so you avoid static initializers, but also initialize the thing in a thread-safe way. But in this case we already know that this function can only be called by a thread that has the GIL, so it's safe to just initialize a global variable without further synchronization. |
fixes
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp | ||
---|---|---|
262–265 | yea | |
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h | ||
755 | I can't use return, but i can have the scripts put their function in a well-known output variable. | |
766–767 | 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. | |
lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp | ||
790–803 | that doesn't seem to support multiple regexes, and is doing it 5 times really any more clear than just doing it manually? |
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.
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h | ||
---|---|---|
755 | 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", ...) | |
766–767 | 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. | |
lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp | ||
790–803 | 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. |
lldb/scripts/Python/python-extensions.swig | ||
---|---|---|
683–686 | You don't have to do this if you don't want, but I couldn't help noticing that the "else" branch is completely unnecessary in all of these scriptlets. | |
lldb/scripts/Python/python-typemaps.swig | ||
4 | Could you delete these empty lines too. You don't have to delete absolutely all of them, if you think they're useful for better visual separation, but I definitely don't think empty lines at the very beginning of a block are useful. | |
lldb/scripts/Python/python-wrapper.swig | ||
193–194 | These even have a double empty line now. :) | |
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp | ||
262–265 | awesome. | |
1059 | Ok, thanks for explaining. The new comment is much clearer now (though I am not sure if it's even needed TBH). | |
1069–1071 | This won't work. The reason why the Twine class is so efficient, is that it establishes links between various temporary objects. Those temporaries will be destroyed when the enclosing full expression is evaluated. This means it's never safe to store Twine objects as local variables. | |
1078 | How likely are we to get another exception while trying to retrieve the backtrace? I am wondering if all this complexity is worth it... | |
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h | ||
26–27 | No using declarations in headers. | |
lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp | ||
790–793 | EXPECT_THAT_EXPECTED(As<long long>(factorial(5ll)), llvm::HasValue(120)) | |
842–843 | HasValue(foobarbaz) |
fixes
lldb/scripts/Python/python-extensions.swig | ||
---|---|---|
683–686 | good point. | |
lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp | ||
1078 | It's actually not that unusual that someone writes a __str__ method for their exception which is broken and also throws an exception. | |
1078 | actually, it looks like traceback.print_exception already takes care of that situation, so you're right it's not very useful. | |
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h | ||
26–27 | even an "Impl" header? |
lldb/scripts/Python/python-wrapper.swig | ||
---|---|---|
232 | There are empty lines from this point on in this file. Before they made sense because they were separating the using directive from the actual code, but now they've just become useless empty lines at the start of a block. Please delete them. | |
lldb/source/Plugins/ScriptInterpreter/Python/ScriptInterpreterPythonImpl.h | ||
26–27 | Nope. The "impl" here comes from the class name, but this is still a header like any other. Theoretically you could also put everything declared in this header into the python namespace, but otoh we have to stop somewhere, and this patch has gotten quite big already. |
I should've mentioned this earlier, but llvm style is to generally use // comments https://llvm.org/docs/CodingStandards.html#comment-formatting.,