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

[Daniel Austin Noland via lldb-dev]

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.

I also find that writing this kind of SFINAE stuff can be difficult and
time consuming.

Do you think this sort of validation is actually an asset (especially
since it will only be available on the private side of things in the
near term)?


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <http://lists.llvm.org/pipermail/lldb-dev/attachments/20160929/0622e5e4/attachment-0001.sig>