labath added a comment.
Hi Abhishek,
Thank you for incorporating the changes. I still see some room for improvement
in the cmake files. I realize that this is something most people are not
familiar with, and is not exactly glamorous work, but it's still part of our
codebase and we should make sure it follows best practices.
================
Comment at: tools/intel-features/CMakeLists.txt:16
+endif()
+
+if (NOT LLDB_DISABLE_PYTHON AND LLDB_BUILD_INTEL_PT)
----------------
Could we avoid building the shared library altogether if both features are OFF?
================
Comment at: tools/intel-features/cli-wrapper.cpp:27
+bool lldb::PluginInitialize(lldb::SBDebugger debugger) {
+ PTPluginInitialize(debugger);
+ MPXPluginInitialize(debugger);
----------------
You will need some ifdef magic to make sure these still compile when the
feature is off.
================
Comment at: tools/intel-features/intel-mpx/CMakeLists.txt:9
+
+set(MPX_DEPS ${MPX_DEPS} LLVMSupport PARENT_SCOPE)
----------------
What you want here is to define an INTERFACE dependency on the MPX library
instead.
vanilla cmake way would be `target_link_libraries(lldbIntelMPX INTERFACE
LLVMSupport)`. **However**, we should use the llvm function instead, as that
also handles other llvm-specific magic (for example, this code will break if
someone does a LLVM_LINK_LLVM_DYLIB build).
So, I am asking for the third time:
Have you tried using add_lldb_library instead?
The correct invocation should be `add_lldb_library(foo.cpp LINK_LIBS Support)`
and the rest of this file can just go away.
================
Comment at: tools/intel-features/scripts/lldb-intel-features.swig:9
+
+/* Various liblldb typedefs that SWIG needs to know about.*/
+#define __extension__ /* Undefine GCC keyword to make Swig happy when
----------------
There appear to be no typedefs here.
================
Comment at: tools/intel-features/scripts/lldb-intel-features.swig:14
+ as INT32_MAX should only be defined if __STDC_LIMIT_MACROS is. */
+#define __STDC_LIMIT_MACROS
+%include "python-typemaps.txt"
----------------
You are already defining this as a part of the swig invocation in cmake.
================
Comment at: tools/intel-features/scripts/python-typemaps.txt:1
+/* Typemap definitions to allow SWIG to properly handle some data types */
+
----------------
abhishek.aggarwal wrote:
> labath wrote:
> > Could we just use standard lldb typemaps? I don't see anything ipt specific
> > here...
> Unfortunately, we can't because that file uses lldb_private::PythonString,
> lldb_private::PythonList etc and for that I will have to link to
> liblldbPluginScripInterpreterPython.
Ok, let's leave this as-is then. We could try factoring out common part of
those typemaps, but I'm not sure that's such a good idea at the moment.
https://reviews.llvm.org/D33035
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
