-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix typos in the codebase #26014
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
Fix typos in the codebase #26014
Conversation
This fixes various typos in the codebase. The work is done using Gemini and manually checked and fixed by me. This does not touch third party code or files with external licenses.
This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (6) test expectation files were updated by running the tests with `--rebaseline`: ``` test/codesize/audio_worklet_wasm.expected.js updated test/codesize/hello_wasm_worker_wasm.expected.js updated test/codesize/test_codesize_file_preload.expected.js updated test/codesize/test_codesize_minimal_O0.expected.js updated codesize/test_minimal_runtime_code_size_audio_worklet.json: 6157 => 6157 [+0 bytes / +0.00%] codesize/test_unoptimized_code_size.json: 180994 => 180979 [-15 bytes / -0.01%] Average change: -0.00% (-0.01% - +0.00%) ```
This does not include llvm-libc changes, which is now synced with the upstream main branch. This includes changes from emscripten-core/emscripten#26014, which has not yet landed.
sbc100
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.
Nice!
This is great. I'm curious how you used the llm to produce these changes. Is this something we can turn into a script to run periodically or on checkin maybe?
Can you split out the changes the effect actual code (I think I've tagged them all), then mark this as NFC in the title?
| break; | ||
| } | ||
| case 0x000330005: { // GLFW_RAW_MOUSE_MOTION | ||
| case 0x00033005: { // GLFW_RAW_MOUSE_MOTION |
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.
This looks different to the other changes in this PR since its actually a bugfix with a functional effect. Can we split this one out into its own change? Is it possible to test is? Maybe we don't have any good coverage of this code?
| default: | ||
| #if OPENAL_DEBUG | ||
| dbg(`alBufferData() called with invalid format ${format}`; | ||
| dbg(`alBufferData() called with invalid format ${format}`); |
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.
This change and the one below are fixes for syntax errors. Can you split these out too?
| // Drop now unneeded references to from the Module object in this Worker, | ||
| // these are not needed anymore. | ||
| props.wasm = props.memMemory = 0; | ||
| props.wasm = props.wasmMemory = 0; |
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.
This is an actual bugfix too. Can you split it out?
| #include <unistd.h> | ||
| #include <fcntl.h> | ||
| #include <fcntl.h> | ||
| #include <unistd.h> |
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.
This is an interesting one. Do you know why it picked up on this but didn't remove unused includes in the rest of the codebase. This somehow seems different fixing typos.
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.
I just asked it to fix typos. I don't know what Gemini was thinking 🤷🏻.
I think Gemini removed them not because they were unused but because they were duplicated with the includes right above them. It didn't need to analyze the whole codebase to check if they were unused. And these two lines are probably too small a change to warrant a separate PR I guess?
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.
Moved to #26024
| elif arg == '-mno-sign-ext': | ||
| feature_matrix.disable_feature(feature_matrix.Feature.SIGN_EXT) | ||
| elif arg == '-mnontrappting-fptoint': | ||
| elif arg == '-mnontrapping-fptoint': |
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.
This looks like a bugfix too.
This does not include llvm-libc changes, which is now synced with the upstream main branch. This includes changes from emscripten-core/emscripten#26014, which has not yet landed.
Unfortunately, it wasn't a magic prompt that we can run periodically. This just started as a fun PR that I thought I would be able to do in a hour or something. First I asked Gemini something like "Fix any typos you find in the codebase, excluding third party code". Then it just searched common misspellings in the codebase using This was done by... asking Gemini to actually read files using "Read ??? and fix all typos you find. Don't just look for commonly misspelled words; actually read each file and fix typos. Read files from start to finish (not just first 2000 lines)." ??? can be a list of of code paths that Gemini can handle at once, like, Anyway this ended up taking a lot longer than expected and I regretted starting it 🫠. I think doing this again is not really a good use of somebody's time. Or in a few years Gemini will be a lot better with 50x context window so maybe we really can do this in a single prompt.
Even after splitting those real code bugfixes out, I'm not sure we can mark this NFC, because there are still many error message fixes in Python files (which are code changes too) and docs changes (.rst files). Will close this and try to split the PR into several categories instead. |
Spun out of emscripten-core#26014. This fixes typos in docs, `settings.js` (because the comments here are copied into `settings_reference.rst`), and header strings in `update_settings_docs.py`.
Spun out of emscripten-core#26014. This fixes typos in docs, `settings.js` (because the comments here are copied into `settings_reference.rst`), and header strings in `update_settings_docs.py`.
This fixes typos in code comments. Split out of emscripten-core#26014.
This fixes typos in error / log messages. Split out of emscripten-core#26014.
This fixes typos in error / log messages. Split out of emscripten-core#26014.
This fixes typos in code that's NFC. This includes cases like typos in method or parameter names or whitespace fixes. This is "mostly" NFC because method name changes in `test_***.py` result in test name changes. Split out of emscripten-core#26014.
Split out of emscripten-core#26014.
This fixes typos in code comments. Split out of #26014.
|
Yes, hopefully soon there will just be bot we can use that can run over all repos that does this kind of thing automatically. |
This fixes typos in docs, `settings.js` (because the comments here are copied into `settings_reference.rst`), and header strings in `update_settings_docs.py`. Split out of #26014.
This fixes typos in error / log messages. Split out of #26014.
This fixes typos in code that's NFC. This includes cases like typos in method or parameter names or whitespace fixes. This is "mostly" NFC because method name changes in `test_***.py` result in test name changes. Split out of #26014.
This fixes various typos in the codebase. The work is done using Gemini and manually checked and fixed by me. This does not touch third party code or files with external licenses.