[Lldb-commits] [PATCH] D69793: Bundle libedit-compatible readline replacement

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks for doing this. I really like how you've implemented this. Just some small stylistic comments inline.



================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonReadline.cpp:1
+#ifdef LLDB_DISABLE_LIBEDIT
+
----------------
Should this also be `#if !APPLE`? IIUC, this is not an issue on apple platforms, and the real readline module is going to be more functional than our mocked up version...


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonReadline.cpp:29-32
+#else
+PyDoc_STRVAR(moduleDocumentation,
+             "Stub module meant to avoid linking GNU readline.");
+#endif
----------------
It looks like this isn't needed then.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonReadline.cpp:82
+PyMODINIT_FUNC initlldb_readline(void) {
+#ifndef LLDB_DISABLE_LIBEDIT
+  PyOS_ReadlineFunctionPointer = simple_readline;
----------------
and this


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonReadline.h:1-2
+//===-- PythonReadline.h -------------------------------------------*- C++
+//-*-===//
+//
----------------
Fix header.


================
Comment at: lldb/source/Plugins/ScriptInterpreter/Python/PythonReadline.h:13-15
+#ifndef LLDB_DISABLE_LIBEDIT
+extern "C" PyMODINIT_FUNC initlldb_readline(void);
+#endif
----------------
How about if this file just exposes a single function like `WorkAroundLibeditIncompatibilityIfNeeded`? Then this function can be just a no-op if no work is needed, and there's no need for messy ifdefs anywhere...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69793