[lldb-dev] RFC: Break/Watchpoint Callback storage and validation

[Zachary Turner via lldb-dev]

On Thu, Sep 29, 2016 at 5:34 PM Daniel Austin Noland via lldb-dev <
lldb-dev at lists.llvm.org> wrote:

> As per discussion in another thread
>
> (http://lists.llvm.org/pipermail/lldb-dev/2016-September/011397.html)
>
> I have started refactoring the private side of Break/Watchpoint.
>
> Mostly this involves fairly trivial and obvious changes and I expect the
> first patch to be ready fairly soon.
>
> Still, there are a couple points I would like to get feedback on.
>
> 1. Having reviewed the llvm::function_ref template (see
> http://llvm.org/docs/doxygen/html/STLExtras_8h_source.html#l00059), I
> can tentatively agree that it is suitable for break/watch callbacks.
>
> The concern is the "This class does not own the callable, so it is not
> in general safe to store a function_ref" bit.  This is not an issue
> provided I ensure that
>
> a) StoppointOptions does "own" the callable OR ensures its validity
> prior to invoking the callback
>
> AND
>
> b) ~StoppointOptions behaves responsibly in the face of this provision
>
> If we are just handed a function pointer then there is nothing to clean up.
>
> If we are handed a callable object things are a little more complex.
>
> We could make a copy of the provided object and just let the dtors clean
> up for us. That does put a few restrictions on the type of callback
> objects we can accept (e.g. user must account for side effects in
> ctor/dtors, object must be copy constructible, performance and memory
> overhead).
>
> We could require move semantics.
>
> We could use unique_ptr, shared_ptr, weak_ptr.
>
> Feelings?
>
> 2. llvm::function_ref and std::function are great, but they accept ANY
> callable.
>
> When I write templates I generally write helper classes which try to
> validate the template params.
>
> Here is a simple sketch of the type of thing I mean:
>
> ```cpp
>
> #include <type_traits>
> namespace callback {
>
> template <class Callable>
> struct IsReturnTypeValid {
>    static constexpr bool value = false;
> };
>
> template <class Ret, class ...OtherParams>
> struct IsReturnTypeValid<Ret(OtherParams...)> {
>    static constexpr bool value = std::is_same<
>                                      typename std::remove_const<Ret>::type,
>                                      bool
>                                   >::value;
> };
>
> template <class Callable>
> constexpr
> void assert_valid_callback() {
>    static_assert(IsReturnTypeValid<Callable>::value,
>                  "Return type of Stoppoint callback must be bool!");
> }
>
> } // end namespace ::callback
>
>
> bool valid(void* userData, int some, double other, char params) {
>    return true;
> }
>
>
> double invalid(int stuff, double moreStuff) {
>    return 0;
> }
>
> int main(void) {
>
>    // compiles
>
>    callback::assert_valid_callback<decltype(valid)>();
>    // will not compile
>
>    callback::assert_valid_callback<decltype(invalid)>();
>    return 0;
> }
> ```
>
> I find that the makes debugging far easier for people using the library.
>
> This seems unnecessary.  If you've got an llvm::function_ref<bool(void*,
int, double, char)> then it's simply not going to compile if you pass it a
mismatched callable.  Is this not sufficient?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20160930/b8b2b568/attachment.html>