[Lldb-commits] [PATCH] D142926: [lldb][RFC] Replace SB swig interfaces with API headers

[Alex Langford via Phabricator via lldb-commits]

bulbazord added inline comments.


================
Comment at: lldb/cmake/modules/LLDBFramework.cmake:91
     DEPENDS ${header} OUTPUT ${staged_header}
-    COMMAND ${CMAKE_COMMAND} -E copy ${header} ${staged_header}
-    COMMENT "LLDB.framework: collect framework header")
+    COMMAND unifdef -USWIG -o ${staged_header} ${header} || (exit 0)
+    COMMENT "LLDB.framework: collect framework header and remove SWIG macros")
----------------
mib wrote:
> May be we should add a cmake check to make sure `unidef` is available on the system.
Yeah for the purposes of this diff I just used it without checking. If we go ahead with this patch I'll add some safety checks.


================
Comment at: lldb/include/lldb/API/SBAddress.h:14-16
+#ifdef SWIG
+%include "interface/SBAddressDocstrings.i"
+#endif
----------------
clayborg wrote:
> Do we want two different .i files? Right now we have both"interface/SB*Docstrings.i" and "interface/SB*Extensions.i". Can those be combined into just one "interface/SBAddress.i" and only included if we have anything in the .i file that isn't just a copy of the API itself? Or does the doc string stuff have to come first?
What I discovered when working this patch is that the docstring information needs to come before the declaration of what it wants to document. So `SB*Docstrings.i` should come first. The `SB*Extensions.i` are there to extend an existing class, so they should come after the class itself. I'm not sure there's a good way to do this.

Alternatively, we could changes the `interfaces.swig` file to include `./interface/SBAddressDocstrings.i`, then `lldb/API/SBAddress.h`, then `./interface/SBAddressExtensions.i`. That way we don't need to have these 2 `#ifdef` blocks in the API headers themselves. 


================
Comment at: lldb/include/lldb/API/SBAddress.h:33
 
+#ifndef SWIG
   const lldb::SBAddress &operator=(const lldb::SBAddress &rhs);
----------------
clayborg wrote:
> Do we need all assignment operators left out for Swig? If so it might be good to add a comment explaining so it is clear to people. I can't remember if this is true or not in all cases?
Yeah I can add a comment. When generating python bindings, swig will ignore assignment operators as it doesn't map cleanly into python, which is why I left this out. It may be more accurate to do something like `#ifndef SWIGPYTHON` but seeing as the `SBAddress.i` class doesn't have an `operator=` in it, I decided to leave it out for swig generation entirely.


================
Comment at: lldb/include/lldb/API/SBAddress.h:133
 
+#ifndef SWIG
 bool LLDB_API operator==(const SBAddress &lhs, const SBAddress &rhs);
----------------
clayborg wrote:
> Do we need all == operators left out of Swig stuff? Comment if so? Can't remember if this applies to all API objects or just to SBAddress?
I don't think that all instances of `operator==` need to be left out. There are some classes that have it in the interface files, e.g. SBBreakpoint. I intentionally left this one out because it wasn't in `SBAddress.i` and I wanted this to leave the python bindings roughly the same (no new functions/methods, re-ordering them in the `lldb.py` should be ok though).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142926