-
-
Notifications
You must be signed in to change notification settings - Fork 180
Fix SECTION UNION alignment depending on piece order
#1883
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
ISSOtm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, fix looks good, and the refactoring doesn't strike me as incorrect, so all good.
|
I'm personally not convinced by the use of a LLM here, since the hard work (of narrowing the bug down) had already been performed, and a human would've been able to perform the fix in the same five minutes. [EDIT: Some friends asked me about this, and I've given a more lengthy analysis of my stance, but wish to reproduce it in a more public place for easier future linking to.] The intelligence and expertise sought out of us developers lies as much in the “I know what I need, so I'm going to do this” as it does in evaluating whether “this” is adapted or not; delegating the actual code modifications to a third party (be it a LLM or an intern, that changes nothing for you) implies detaching yourself from them, and thus stop evolving your understanding of the code—thus losing your expertise through aging, like a manager. For context to the above PR and linked issue, Rangi raised the original bug in polishedcrystal to me yesterday, asking for help narrowing it down; I replied “I think the error is in this component1, here's a sequence of actions we can take to verify that (and thus pare down how much code may be involved and where), huh they are consistent, thus the error must be in another component, 🥱 well let's investigate the rest tomorrow”. Had we delegated writing the code entirely to a LLM, the codebase would be a black box to us, and thus I would've been unable to provide the above reasoning. At best, I could've thrown the black box at another one, asking a LLM to analyse the codebase and hope that its analysis made sense. Unless one trains a LLM on a specific codebase, it's never going to gain specific domain knowledge, and thus is far more likely to answer incorrectly2, and you'd be none the wiser. Where it all gets to a head is partial bug fixes: for example, if the LLM had judged that a And, yes, the exact one-liner above is clearly wrong to at least this project's two maintainers; but, increase the complexity of the putative submitted patch a little, or decrease your knowledge of the codebase (such as by the “black-boxing” process I outlined in an earlier paragraph), and judging whether a solution is “truly” appropriate becomes much, much more tricky. tl;dr: A LLM does not learn from repeated interactions, while delegating the actual work of typing the words and laying out the logic ourselves creates distance from the code (more as a result of losing subconscious / “muscle-memory” engagement) that weakens our very ability to oversee and evaluate the LLM's output. I would parallel this to what I've heard about “learning to type before to hand-write reduces your understanding of language”, and also the more general sentiment that LLM usage amounts to “rolling up the ladder behind us”. Footnotes
|
If you mean that you're not convinced it's a correct or complete bug fix, that's what PR review is for anyway -- if/when I'd come across this one-line discrepancy in manual code debugging, I can tell that I'd believe it to be correct, and would have opened this same PR no matter what. So regardless of LLMs, I'm looking for a second pair of eyes to verify that.
I'm... not going to get into that. 😅 I don't think GitHub Issues is the place for it, and the only reason I mentioned Codex at all was to give vulcandth proper credit, and to avoid claiming/implying that either of us personally read and understood the alignment-related code well enough to have found this. (I know I'd have taken more than five minutes investigating other possibilities before landing on this correct one.) Maybe in future I should just not mention if/how AI was involved, if it just causes unnecessary controversy over a one-line bugfix (and subsequent refactor that's entirely my judgement). Edit:
Oh okay, so you are convinced it's a good fix. Then I don't think anything else is relevant, at least not to be discussed in this issue. (I suppose if you want it to be in "a more public place", a GitHub Discussion would also work, if you want to move your stance description to there? Or your blog, if you feel like it's more personal-belief-related than RGBDS-maintenance-related.) |
c42a49d to
d075253
Compare
Fixes #1881
The one-line fix was found by @vulcandth using OpenAI Codex (in 5 minutes!), prompted with the issue description I had written. Its conclusion:
I'm pretty confident this is a correct fix, given that it now makes
checkSectUnionCompatclearly symmetrical withcheckFragmentCompat. I can add more test cases if we think of any that would cover appropriate new scenarios.Also confirmed that it builds https://github.com/ariscop/polishedcrystal/tree/audio even with an
align 8now.