[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/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