[Lldb-commits] [lldb] Allow multiple destroy callbacks in `SBDebugger::SetDestroyCallback()` (PR #89868)

[Jonas Devlieghere via lldb-commits]

https://github.com/JDevlieghere requested changes to this pull request.

Can you elaborate a bit more about why you want to change the behavior? Your motivation touches on the fact that it might be surprising or racy. While I think having the ability to register multiple callbacks makes sense, I'm not sure I agree with either.

 - The method is called `SetDestroyCallback` which makes it pretty clear that it's a setter, which is generally expected to change the value, rather than add to it. If the method had been called `AddDestroyCallback` on the other hand, I would expect it to add the callback rather than overwrite it. 
 - I don't think there's anything racy about a setter. Even if it were, nothing in this PR addresses the racy-ness.

The setter also makes it possible to remove a callback by overwriting it with a callback that does nothing. With the current approach that's no longer possible. I'm not sure if that's important, but something to consider. 

If the goal is to be able to specify multiple callbacks, a potential solution could be to store the callbacks in a list as you do in this PR, but add a new method `AddDestroyCallback` which appends to the list. We could keep the existing behavior for `SetDestroyCallback` by clearing the list and adding it as the first entry. That would also solve the problem of not being able to remove existing callbacks. 

Regardless of the direction, this definitely needs a corresponding test and updated documentation. 

https://github.com/llvm/llvm-project/pull/89868