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

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

In D142926#4113600 <https://reviews.llvm.org/D142926#4113600>, @clayborg wrote:

> In D142926#4111717 <https://reviews.llvm.org/D142926#4111717>, @bulbazord wrote:
>
>> Certainly interesting:
>>
>> - SBListener::StopListeningForEventClass return type conflicts (ABI break?)
>
> No, probably a copy paste error when originally checked into .i file?

Likely. This method is already tested in TestListener.py. Previously it returns an uint32_t (which turns into a python integer), now it returns a bool.

>> - SBProcess::GetQueueAtIndex parameter types conflict (ABI break?)
>
> No, probably a copy paste error when originally checked into .i file?

Almost certainly. This method is well tested, I don't have many concerns here.

>> - SBTarget::BreakpointCreateByNames first parameter is different: `const char **symbol_name` vs `const char *symbol_name[]`. I'm not sure if this is going to be an issue.
>
> Maybe SWIG didn't like the "const char **" or it didn't do the right thing? Test and verify if we switch to using the header file we have no issues please?

I'm not 100% sure why there was some divergence. For now I've done some `ifdef/else/endif` to separate this. I don't think there's a problem if we pick one over the other but this method is completely untested so I am punting it a bit. :)

In D142926#4113615 <https://reviews.llvm.org/D142926#4113615>, @clayborg wrote:

> In D142926#4111717 <https://reviews.llvm.org/D142926#4111717>, @bulbazord wrote:
>
>> Potentially interesting:
>>
>> - SBData::GetDescription base_addr parameter has default value now
>> - SBInstructionList::GetInstructionsCount canSetBreakpoint has default value now
>> - SBMemoryRegionInfo::SBMemoryRegionInfo 3rd constructor parameter stack_memory has default value now
>
> Make sure it does work. I seem to remember people sometimes making two functions in the .i file for default parameters. Can't remember if SWIG does the right thing with default params or not, but please test to ensure both work (with and without).

I went through all 3 of them myself. Their result of having a default argument now is that there is a version of the function exposed that has 1 less argument. For example, `SBInstructionList::GetInstructionsCount` has 3 arguments where the last one now has a default value. You can still do `GetInstructionsCount(arg1, arg2, arg3)` where arg3 can be `True` or `False`. This is the same behavior as before. The new thing that got added was `GetInstructionsCount(arg1, arg2)` where arg3 will default to `False` without you specifying it. This is true for `SBData::GetDescription` and `SBMemoryRegionInfo`'s 3rd constructor.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142926