JDevlieghere added a comment. In D77759#1992252 <https://reviews.llvm.org/D77759#1992252>, @labath wrote:
> In D77759#1988419 <https://reviews.llvm.org/D77759#1988419>, @labath wrote: > > > Hmm... well, I like how this (active&passive) is unified at the template > > level. It still feels like there's too much code to implement this thing > > (coming from the duplication for const, non-const and static variants), but > > I'm not sure what can be done about that. I'll need to ponder this a bit > > more > > > After sleeping on this a couple of times, I've come to believe that the > problem here is that we've let the redirection functions inherit the c++ > function pointer weirdness, which resulted in a need to write template goo > for const, non-const and static methods (and maybe sometimes even > constructors). The goo is (more or less) unavoidable if we want to have > compiler-generated "identifiers" for all these functions, but there's a > difference between writing that once, and once for each redirection we need > to make. Particularly as it doesn't seem like there's a compelling reason for > that. There no reason that the "redirected" function in the replayer > machinery needs be globally unique in the same way as the "original" > functions do. In fact I think it doesn't even have to be a template at all -- > it could just take the function-to-wrap as a regular argument instead of a > templated one. Yeah, I've come to the same conclusion while working on this. Hindsight is 20/20 and changing all that might be a significant amount of work. > I believe `char_ptr_redirect<goo>::method<&goo>::record` could have been > written as: > > // this handles const and non-const methods > template<typename Result, typename This> // Maybe "Result" not even needed, > if this is always the same type. > Result char_ptr_redirect(Result (*f)(This *, char *, size_t), This *this_, > char *ptr, size_t len) { > char *buffer = reinterpret_cast<char *>(calloc(len, sizeof(char))); > return f(this_, buffer, len) > } > // and another overload without This for static functions > > > Similar thing could be applied to the "replay" functions introduced here. > Reduction from three overloads to two may not sound like much, but given that > it would also remove the need for a lot of template goo (and reduce code > size), I still find it interesting. It might be possible to also merge the > two remaining overloads if we try hard enough. Agreed and while it would be a small thing it would also help compile times: this file is by far the slowest to compile in lldb. That and the templates have a significant cognitive overhead. > Nonetheless, as long as we have only one kind of redirects (char_ptr thingy), > it's benefit is not very clear, so I'm not going to insist on that. However, > if more of these crop up, I think we should definitely revisit this. Thanks, I totally share your concerns and really appreciate the pragmatism. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77759/new/ https://reviews.llvm.org/D77759 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
