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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
