abhishek.aggarwal added a comment.
Hi Pavel .. I have made the changes you suggested. My apologies for
misinterpreting your previous comments but during written communications, it is
sometimes difficult to interpret everything correctly. I have tried following
LLDB's coding conventions and guidelines. Please let me know if I still missed
things that you would have liked to see in this patch. Thanks for your patience
:)
================
Comment at: tools/intel-features/CMakeLists.txt:16
+endif()
+
+if (NOT LLDB_DISABLE_PYTHON AND LLDB_BUILD_INTEL_PT)
----------------
labath wrote:
> Could we avoid building the shared library altogether if both features are
> OFF?
yes. Adding the check.
================
Comment at: tools/intel-features/cli-wrapper.cpp:27
+bool lldb::PluginInitialize(lldb::SBDebugger debugger) {
+ PTPluginInitialize(debugger);
+ MPXPluginInitialize(debugger);
----------------
labath wrote:
> You will need some ifdef magic to make sure these still compile when the
> feature is off.
Fixed it.
================
Comment at: tools/intel-features/intel-mpx/CMakeLists.txt:9
+
+set(MPX_DEPS ${MPX_DEPS} LLVMSupport PARENT_SCOPE)
----------------
labath wrote:
> 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.
I am extremely sorry Pavel but I understood it now what you were trying to say
in previous comments. Sorry about misinterpreting your comments before. I have
used add_lldb_library function now. Please see them in the next patch set.
================
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
----------------
labath wrote:
> There appear to be no typedefs here.
Forgot to remove this. Doing it.
================
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"
----------------
labath wrote:
> You are already defining this as a part of the swig invocation in cmake.
removing it.
https://reviews.llvm.org/D33035
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
