This is an archive of the discontinued LLVM Phabricator instance.

Add "operator bool" to SB APIs
ClosedPublic

Authored by labath on Feb 28 2019, 12:50 PM.

Details

Summary

Our python version of the SB API has (the python equivalent of)
operator bool, but the C++ version doesn't.

This is because our python operators are added by modify-python-lldb.py,
which performs postprocessing on the swig-generated interface files.

In this patch, I add the "operator bool" to all SB classes which have an
IsValid method (which is the same logic used by modify-python-lldb.py).
This way, we make the two interfaces more constent, and it allows us to
rely on swig's automatic syntesis of python nonzero methods instead
of doing manual fixups.

Diff Detail

Repository
rL LLVM

Event Timeline

labath created this revision.Feb 28 2019, 12:50 PM
shafik added a subscriber: shafik.Feb 28 2019, 1:53 PM

It stood out to me that some of the conversions were not const and I can see that IsValid is not consistently const across the API but after talking to @jingham it is unfortunately something we can't change.

labath added a comment.Mar 1 2019, 5:40 AM

It stood out to me that some of the conversions were not const and I can see that IsValid is not consistently const across the API but after talking to @jingham it is unfortunately something we can't change.

Yes, that is unfortunate. I can think of three things that we could do differently though:

  1. add a const version of IsValid where it is missing, and have and always-const operator bool which uses that
  2. give up on constness and just have a non-const operator bool everywhere
  3. add a const operator bool everywhere, and have IsValid (const or non-const) call into that

Each option has different tradeoffs, and it's not really clear to me which one is better. I am happy to implement whichever you think is best.

It stood out to me that some of the conversions were not const and I can see that IsValid is not consistently const across the API but after talking to @jingham it is unfortunately something we can't change.

Yes, that is unfortunate. I can think of three things that we could do differently though:

  1. add a const version of IsValid where it is missing, and have and always-const operator bool which uses that
  2. give up on constness and just have a non-const operator bool everywhere
  3. add a const operator bool everywhere, and have IsValid (const or non-const) call into that

"const" is useless when your class contains a shared pointer or a unique pointer. It also changes the API mangling which we can't do. So I vote for give up on const as you can call non const methods in any shared and unique pointers because const is only protecting the pointer from changing.

Each option has different tradeoffs, and it's not really clear to me which one is better. I am happy to implement whichever you think is best.

labath updated this revision to Diff 188966.Mar 1 2019, 1:28 PM

Give up on constness and make all bool operators non-const.

shafik added a comment.Mar 1 2019, 2:29 PM

It stood out to me that some of the conversions were not const and I can see that IsValid is not consistently const across the API but after talking to @jingham it is unfortunately something we can't change.

Yes, that is unfortunate. I can think of three things that we could do differently though:

  1. add a const version of IsValid where it is missing, and have and always-const operator bool which uses that
  2. give up on constness and just have a non-const operator bool everywhere
  3. add a const operator bool everywhere, and have IsValid (const or non-const) call into that

"const" is useless when your class contains a shared pointer or a unique pointer. It also changes the API mangling which we can't do. So I vote for give up on const as you can call non const methods in any shared and unique pointers because const is only protecting the pointer from changing.

Each option has different tradeoffs, and it's not really clear to me which one is better. I am happy to implement whichever you think is best.

The goal of const is to communicate to the user that it does not mutate the underlying object, which in general is good practice to maintain if possible.

In this case I don't see any good trade-offs here. I don't feel like adding a const version along w/ a non-const version really helps since it still muddies the waters.

Out of curiosity, are there known, specific examples of users who rely on the exact mangling not changing?

If this is new API I guess it is ok to have them all be const. I was mostly worried that you wouldn't be able to call a non const function from a const function. Do we have any IsValid() calls that are const? I believe we do. If so, how are we calling IsValid() if it is const from a non const conversion operator?

Out of curiosity, are there known, specific examples of users who rely on the exact mangling not changing?

Yes, both Xcode and lldb-rpc-server (an out-of-process dingus we made for Xcode) build against some particular version of the SB headers, but then expect to be able to load many different versions of LLDB.framework.

Out of curiosity, are there known, specific examples of users who rely on the exact mangling not changing?

yes. Xcode. For Swift, Xcode runs the LLDBRPC.framework in process which runs lldb-rpc-server out of process with the DYLD_FRAMEWORK_PATH set to the directory that contains the LLDB.framework that lldb-rpc-server wants to run with. API is very important and without it this would not have been possible. So API is very important.

That being said, at some point we should make a lldb2 namespace and make a liblldb2.so and cull out all of the fluff we are keeping in there and get down to the exact API we want. We can then change things to be const where needed. remove redundant calls. In the name of backward compatibility we have functions that didn't return an error which now do so we have two versions of similar cal, one with error reporting and one without. Or the 5 flavors of CreateTarget in SBDebugger. There are many others. So at some point it would be nice to go through and reduce and improve the API. That will take time though cause if we do it, we should get it right.

Out of curiosity, are there known, specific examples of users who rely on the exact mangling not changing?

yes. Xcode. For Swift, Xcode runs the LLDBRPC.framework in process which runs lldb-rpc-server out of process with the DYLD_FRAMEWORK_PATH set to the directory that contains the LLDB.framework that lldb-rpc-server wants to run with. API is very important and without it this would not have been possible. So API is very important.

That being said, at some point we should make a lldb2 namespace and make a liblldb2.so and cull out all of the fluff we are keeping in there and get down to the exact API we want. We can then change things to be const where needed. remove redundant calls. In the name of backward compatibility we have functions that didn't return an error which now do so we have two versions of similar cal, one with error reporting and one without. Or the 5 flavors of CreateTarget in SBDebugger. There are many others. So at some point it would be nice to go through and reduce and improve the API. That will take time though cause if we do it, we should get it right.

This would be a fun project, however we should not design this by ourselves. Though we are responsible for the SB API, and those of us who write dotest tests frequently use it some, I bet none of us has written a big application using it. Greg's probably the closest, with the heap.py and so forth scripts. However, there are folks out there who have used it extensively. At Apple, the kernel folks have written a pretty big set of utilities based on the Python API's, and of course the Xcode folks have done so using the C++ API's as well. And there are likely other folks around who have been using it more extensively. We could learn a lot from them.

Another really good way to drive the design process by actual example would be to take the opportunity of coming up with the SB2 API's to rewrite everything in source/Commands on top of the new API's. There are other reasons for doing this (removing duplicated code and testability) but I'm also sure we'd get a lot of good insights into how the design should go doing that.

labath updated this revision to Diff 189057.Mar 2 2019, 12:28 PM

The version which makes "operator bool" const.

If this is new API I guess it is ok to have them all be const. I was mostly worried that you wouldn't be able to call a non const function from a const function. Do we have any IsValid() calls that are const? I believe we do. If so, how are we calling IsValid() if it is const from a non const conversion operator?

The easiest way to fix that is to invert the calling direction. We can make the "operator bool" (which I guess we have agreed we want to be const?) contain the "canonical" implementation of the validity check, and then have the IsValid function (const or non-const) call into that. Since the bool operator will always be const, this will work regardless of whether the IsValid operation is declared to be const or not. If we then wanted to, we could make the "IsValid" function deprecated and remove it in the hypothetical v2 API.

That latest version of the patch implements that idea (the first part of it, the APIv2 thingy is just an idea).

clayborg accepted this revision.Mar 5 2019, 2:55 PM
This revision is now accepted and ready to land.Mar 5 2019, 2:55 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2019, 6:58 AM