[Lldb-commits] [PATCH] D68546: remove FILE* usage from ReportEventState() and HandleProcessEvent()

Tue Oct 8 12:31:35 PDT 2019

lawrence_danna added a comment.

In D68546#1699397 <https://reviews.llvm.org/D68546#1699397>, @labath wrote:

> Yeah, the %extends are somewhat ugly, but I also find the FileSP overloads in the "public" sections of the SB classes unnerving too, because they are not really public -- no user can actually call them directly in a meaningful way. They are really only meant for the python interface, which sort does sit on the other side of the SB api, but also kind of doesn't. That's the weird part about lldb python support, where python can be embedded *into* lldb, but it also can be sitting *above* it (or both at the same time). I am not really sure what to do about that. I don't suppose there's any way to make the FileSP overloads private?

I don't think so, because the SWIG wrappers need to see them.   They are de-facto private though, because they require an argument of type `std::shared_ptr<lldb_private::File>`.
All external API clients can see is a forward declaration that lldb_private::File is a class.   They can make a NULL pointer to one, but that's it.

> Or maybe they actually should be public, and the actual problem is that there is no way for a c++ user to make use of this functionality, even though it should be able to? Right now, the c++ api is sort of disadvantaged in that one cannot create an SBFile that would redirect to some custom fancy c++ output object. I am not saying you should implement that, but if we assume that such functionality were available do you think it would be possible to implement the python stuff on top of that? In that sense, maybe the FileSP overloads are actually fine, and we can treat them as a placeholder for this hypothetical future functionality?
> 
> I am sorry if this doesn't make much sense -- that's because I myself am not clear on this matter. But I hope that I at least managed to give you an idea of the things going through my head right now. If you have any thoughts on any of this, I'd like to hear them.

No, it does make sense.   I too thought it was kind of odd that there's a python API which is inaccessible from C++.      I can't really think of any ultimate use case where a C++ binding for File I/O callbacks would be needed, but if it were, I can think of three ways to do it.

The first and simplest way to support it is to just use `funopen()` or whatever the equivalent of that is on your platform to make a `FILE*`.    I put the `FILE*` constructor in for C++ even though it will never have a SWIG wrapper so this would be possible.    The disadvantage is that `funopen()` isn't portable, so clients that go this route will need 
platform specific code to do it.

The intermediate method would be to make our own funopen that would act as a factory function for `FileSP`.     File would remain private, but you could make one with callbacks if you wanted to.    Disadvantage here is that C++ clients would be calling though a chain of two function pointers to actually get to the Read and Write methods.   This is still better from a performance perspective than calling into a Python method, but if we wanted to avoid that, we could go one step further.

The most complicated way we could handle it would be to expose an ABI-stable abstract base class of `File` in the API headers.     This should be at the base of the File class hierarchy, where `IOObject` is now.    It would only have virtual methods, and it would also have a bunch of unused dummy virtual so we can add new virtual methods later if we need too, without changing the ABI.     C++ clients could then just subclass their own `File` types and use `FileSP` directly.     I think to do this properly we'd need to do some more refactoring, because I'm not sure what should happen to `IOObject` and `Socket` under this scenario, or what kinds of error codes the virtual methods should be returning.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68546