clayborg added inline comments.
================ Comment at: lldb/include/lldb/API/SBAddress.h:14-16 +#ifdef SWIG +%include "interface/SBAddressDocstrings.i" +#endif ---------------- bulbazord wrote: > 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. Anything we can do to keep the headers clean would be great. I know that was the original reason we started using them. ================ Comment at: lldb/include/lldb/API/SBAddress.h:33 +#ifndef SWIG const lldb::SBAddress &operator=(const lldb::SBAddress &rhs); ---------------- bulbazord wrote: > 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. Will it just ignore these if they are left in, or will it end up doing the wrong thing? Anything we can do to keep the header files clean is good. No worries if we have to keep the #ifndef stuff ================ Comment at: lldb/include/lldb/API/SBAddress.h:133 +#ifndef SWIG bool LLDB_API operator==(const SBAddress &lhs, const SBAddress &rhs); ---------------- bulbazord wrote: > 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). > Again, cleaner headers would be nice if we can get away with removing the #ifndef stuff 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
