steakhal wrote:

I'm yet to review the PR, but I would express my opinion on the ergonomics of 
the StreamChecker, as I've spent the last couple of days around it.

I find code duplication less harmful than unnatural generalization over small 
set of functions (I know, it's a hot take :D).
`std::bind` is harmful, and specific handlers should be preferred. As a rule of 
thumb, if we need a flag on a top-level API, then that's likely a code-smell.

E.g.: There presence of `IsRead` or `IsSingleChar` (that we need to bind), 
signals to me that there should be a specific API for the read, write, 
singleChar and buffer cases. Internally, those could dispatch to a generic 
call, which has this flag - but at least on the use-sites, we don't need to use 
`std::bind`, but naturally just taking the address of the member function 
without any tricks.

I'd also advise against using more callables bundled with CallDescriptions. 
They make debugging code more difficult, as the control-flow would become also 
data-dependent. I'd suggest other ways to generalize.
If I have some spare time in the next days, I'm going to also look deeper and 
come up with actual recommendations.

https://github.com/llvm/llvm-project/pull/79312
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to