perl5 icon indicating copy to clipboard operation
perl5 copied to clipboard

toke.c: S_intuit_more: Cleanup

Open khwilliamson opened this issue 2 years ago • 3 comments

"This is terrifying code"

At $work there was a story about hardware that collectively handled perhaps billions of phone calls a day. The interrupt handler was long spaghetti code which had a single comment /* Subtle */. Everyone was afraid to touch it, instead creating workarounds for its bugs. Eventually Management decided to spend the money to rewrite it.

I think this is similar to a portion of S_intuit_more in toke.c, as expressed in #16478. I have spent some hours going through the code, commenting it and adding white space to make it more legible. In the process, I discovered some wasted effort, and some missing things. This PR should make it less terrifying for people to work on

khwilliamson avatar Apr 10 '22 18:04 khwilliamson

I did not analyze why the particular weight factors were chosen. And #16478 indicates this doesn't work as well as claimed

khwilliamson avatar Apr 10 '22 19:04 khwilliamson

I'm a bit concerned that this isn't just cleanup, but also makes a variety of functional changes. Can we list all those in one place (perhaps in the ticket), so that we have the chance to more carefully consider which patterns will be affected by them, and thus what sort of breakage this might lead to in the wild?

I share this concern, and I think the functional changes should be a separate follow-on PR from the cleanup, so that we can confirm that the cleanup didn't break anything before we merge the changes.

Also, some tests demonstrating the changed behaviour would be nice.

ilmari avatar Apr 11 '22 09:04 ilmari

Yes the cleanup should be split from any changes

But now that I understand it better, I see how brittle it is, and that some tests pass just because the arbitrarily chosen numbers happened to work.

One thing that I see missing is context. If this is called immediately when parsing $a[...] it would be helpful for this routine to know that there exists a @a which would lead the initial bias more towards a subscript

I'd like to know why we aren't getting more reports of failures from the field

khwilliamson avatar Apr 11 '22 19:04 khwilliamson

The original pull request is now replaced by other commits which don't change the current behavior

khwilliamson avatar Jul 17 '23 15:07 khwilliamson