bulbazord added inline comments.
================ Comment at: lldb/include/lldb/API/SBAddress.h:14-16 +#ifdef SWIG +%include "interface/SBAddressDocstrings.i" +#endif ---------------- clayborg wrote: > 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. I'll try to minimize the noise in the API headers when I do the complete thing. ================ Comment at: lldb/include/lldb/API/SBAddress.h:33 +#ifndef SWIG const lldb::SBAddress &operator=(const lldb::SBAddress &rhs); ---------------- clayborg wrote: > 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 It ignores them and emits a warning. I suppose we don't care about these specific warnings and swig has a way of disabling specified warnings so I'll just do that instead. ================ Comment at: lldb/include/lldb/API/SBAddress.h:133 +#ifndef SWIG bool LLDB_API operator==(const SBAddress &lhs, const SBAddress &rhs); ---------------- clayborg wrote: > 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 I'm going to keep this one for now. This change is supposed to be relatively NFC and I don't want to introduce extra functionality in the python bindings at the same time we do this. We can re-evaluate adding this to the python bindings at a later time. 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
