[clang] [analyzer] Fix false positive in strchr/strrchr with constant args (PR #210154)

[Balázs Benics via cfe-commits]

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