[Lldb-commits] [PATCH] D71232: [lldb/Lua] Add Boilerplate for a Lua Script Interpreter

Tue Dec 10 00:34:31 PST 2019

labath added inline comments.

================
Comment at: lldb/cmake/modules/LLDBConfig.cmake:28
 set(default_disable_python OFF)
+set(default_disable_lua OFF)
 set(default_disable_curses OFF)
----------------
I think this will tick off some bots (and people) because it means that the default configuration will not build unless one has (compatible?) lua installed. Though I don't really like that, the usual way to handle external dependencies in llvm is to detect their presence and automatically disable the relevant functionality.

Now, that's not how things work in lldb right now, so it _may_ make sense to do the same for lua (though it also may make sense to port everything to the llvm style). However, the current lldb behavior has been a source of friction in the past and I suspect a fresh build error might reignite some of that.

Anyway, you have been warned...

================
Comment at: lldb/cmake/modules/LLDBConfig.cmake:332
+if (LLDB_DISABLE_LUA)
+  add_definitions( -DLLDB_DISABLE_LUA )
+endif()
----------------
Can we replace this (and maybe python too, while at it) with a Host/Config.h entry? A global definition means that one has to recompile everything when these change in any way, whereas in practice only a handful of files need this..

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

https://reviews.llvm.org/D71232