steakhal wrote: I tried a couple more design alternatives and I'm not really satisfied with any of them. My conclusion was that probably the cleanest is the back matching as proposed here - so I think maybe the best course of action is to continue investing here.
At glance, the code looks correct. However, I think we could improve the readability by hoisting some utility lambdas out (if possible). I think as a rule of thumb a lambda should fit on a screen. Nested lambdas can sometimes work, but otherwise they also reduce readability, so should be considered as a last resort IMO. Final note about the control flow of the code that does the `addTransition` calls. Those are critical for correctness, thus I'd suggest to follow a simple control-flow around those. In particular, the `Found branch — skip when the match is impossible.` case has the transition and then returns. This makes sense. Then we have a different set of branches, and have a transition and then an `else`. It seems to me that right before that `else`, we could have a `return` making sure that the transition was the last thing we did. I know that there is no code after the whole `if` block, but having consistency here is critical for readability. In general, fallthrough if/else branches are harmful for readability because one needs to reason about paths and potentially intersecting conditions, so the more declarative we can make this the better. https://github.com/llvm/llvm-project/pull/210154 _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
