Am 07.01.22 um 20:33 schrieb Taylor Blau: > On Fri, Jan 07, 2022 at 01:57:17PM +0100, René Scharfe wrote: >> Am 06.01.22 um 20:50 schrieb Taylor Blau: >>> René, >>> >>> Here are a couple of extra patches on top of your series which introduce >>> and use a new grep_and_expr() function. >>> >>> Like the final patch says, this isn't about reducing code duplication, >>> but rather about adding visual consistency with the other >>> `compile_pattern_xyz()` functions. >>> >>> Taylor Blau (2): >>> grep: extract grep_binexp() from grep_or_expr() >> >> I considered extracting such a function as well. I'd have called it >> grep_binary_expr(), though, to match the existing names. >> >> I decided against it because it can be misused by passing a non-binary >> kind to it. (That's a weak objection, but the benefit of such a >> function was low already in my mind because it doesn't do much.) You >> solve this by keeping grep_or_expr() and adding grep_and_expr(), which >> cannot be misused in this way -- OK. That "OK" should probably have been a "Good". > > That makes sense. If it's keeping you up at night, we could easily add a > check to ensure that `kind` is one of GREP_NODE_OR or GREP_NODE_AND. But > I think that any new code that looks like: > > grep_binexp(GREP_NODE_NOT, xyz, NULL); > > would probably stick out like a sore thumb. So I doubt that such a check > would buy us much practically speaking. Having the wrappers for all the binary node types is good enough. I'm not so sure about the sticking out thing -- I managed to miss bigger mistakes before.. > But I agree that this whole thing probably isn't worth the minimal > effort required, since the couple of patches I posted on top are purely > about cosmetics. > > All of that is to say that I'd be happy to see these patches picked up, > and I would also not be sad at all to see them left on the floor. I think the end result is an improvement. René
