This is an archive of the discontinued LLVM Phabricator instance.

[clang] Fixed a number of typos
ClosedPublic

Authored by GabrielRavier on Jul 30 2022, 3:38 PM.

Details

Summary

I went over the output of the following mess of a command:

(ulimit -m 2000000; ulimit -v 2000000; git ls-files -z | parallel --xargs -0 cat | aspell list --mode=none --ignore-case | grep -E '^[A-Za-z][a-z]*$' | sort | uniq -c | sort -n | grep -vE '.{25}' | aspell pipe -W3 | grep : | cut -d' ' -f2 | less)

and proceeded to spend a few days looking at it to find probable typos
and fixed a few hundred of them in all of the llvm project (note, the
ones I found are not anywhere near all of them, but it seems like a
good start).

Diff Detail

Event Timeline

GabrielRavier created this revision.Jul 30 2022, 3:38 PM
Herald added a reviewer: dang. · View Herald Transcript
Herald added a reviewer: NoQ. · View Herald Transcript
Herald added a reviewer: ributzka. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
GabrielRavier requested review of this revision.Jul 30 2022, 3:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2022, 3:38 PM

Hi, I think the main issue is that your patch contains another patch unintentionally, please fix that :)

clang/CMakeLists.txt
513 ↗(On Diff #448815)

Why delete these lines?

clang/include/clang/Basic/JsonSupport.h
109 ↗(On Diff #448815)

This is not a typo, right? And I think using llvm::is_contained is more impressive?

clang/include/clang/Sema/Sema.h
4022

Normally we don't clang-format legacy code like this, as it's bad for git blame.

clang/lib/Format/Format.cpp
1724 ↗(On Diff #448815)

ditto

clang/lib/Headers/opencl-c.h
17856

Not sure if we want to format this...

clang/lib/Sema/SemaDeclAttr.cpp
1880 ↗(On Diff #448815)

ditto

clang/test/CMakeLists.txt
154 ↗(On Diff #448815)

Why drop this? I thought it was intentional?

clang/test/Interpreter/code-undo.cpp
3 ↗(On Diff #448815)

why add this in a typo-fix patch?

clang/test/Interpreter/execute-weak.cpp
4 ↗(On Diff #448815)

why add this in a typo-fix patch?

clang/test/Interpreter/execute.cpp
5 ↗(On Diff #448815)

why add this in a typo-fix patch?

clang/test/Interpreter/global-dtor-win.cpp
1 ↗(On Diff #448815)

Is this in main?

clang/tools/CMakeLists.txt
17 ↗(On Diff #448815)

Why drop this?

clang/tools/clang-repl/ClangRepl.cpp
31 ↗(On Diff #448815)

I think this patch contains another patch...

70 ↗(On Diff #448815)

I think this patch contains another patch...

130 ↗(On Diff #448815)

I think this patch contains another patch...

clang/unittests/CMakeLists.txt
38 ↗(On Diff #448815)

Why drop this?

clang/unittests/Interpreter/ExceptionTests/InterpreterExceptionTest.cpp
116 ↗(On Diff #448815)

I think this patch contains another patch...

Ooh yeah looks like there's some damage. Shouldn't have done this at 12:30 AM without looking at what that git rebase on the latest pull ended up doing... I'll go fix this asap

GabrielRavier added inline comments.Jul 31 2022, 3:59 AM
clang/lib/Headers/opencl-c.h
17856

Well, it is what clang-format did, so I don't really know... Is it also legacy code as mentioned above ?

Fixed it so that it doesn't accidentally revert all the changes made since my original diff

clang/include/clang/Sema/Sema.h
4022

I've undone the clang-formatting on this

junaire added inline comments.Jul 31 2022, 4:12 AM
clang/include/clang/Sema/Sema.h
4022

Oh, I'm sorry about it I think I missed something here. So basically if you touched the code then format it is a good choice. I thought there was nothing changed here but only formatting, that's something we usually want to avoid.

Reverted reversion of git clang-format on clang/include/clang/Sema/Sema.h

GabrielRavier added inline comments.Jul 31 2022, 7:40 AM
clang/include/clang/Sema/Sema.h
4022

I've un-undone the clang-formatting on this

Thanks for the typo fixes! All the changes look correct to me, so LGTM. Do you need someone to land this on your behalf? If so, what name and email address would you like used for patch attribution?

clang/lib/Headers/opencl-c.h
17856

I think it's fine to reformat this given that the code is being updated. I validated that the only identifiers changed here are parameter names (so there's no chance to break user code).

aaron.ballman accepted this revision.Aug 1 2022, 10:01 AM
This revision is now accepted and ready to land.Aug 1 2022, 10:01 AM
GabrielRavier added a comment.EditedAug 1 2022, 10:10 AM

Do you need someone to land this on your behalf?

I assume so since landing seems to mean pushing the commit to the repo which I'm pretty sure I don't have the rights to do.

If so, what name and email address would you like used for patch attribution?

Gabriel Ravier, gabravier@gmail.com

(By the way, is there any way to make this information part of my Phabricator account so that I don't need to repeat this every time I submit a patch ? The patch I made for clang-tools-extra got in with the right name/email pair just fine without me having to say anything, so I was assuming it was there somewhere...)

This revision was landed with ongoing or failed builds.Aug 1 2022, 10:13 AM
Closed by commit rG5674a3c88088: Fixed a number of typos (authored by GabrielRavier, committed by aaron.ballman). · Explain Why
This revision was automatically updated to reflect the committed changes.

Do you need someone to land this on your behalf?

I assume so since landing seems to mean pushing the commit to the repo which I'm pretty sure I don't have the rights to do.

Okie dokie! I try to double-check with anyone who I'm not used to seeing commits from.

If so, what name and email address would you like used for patch attribution?

Gabriel Ravier, gabravier@gmail.com

(By the way, is there any way to make this information part of my Phabricator account so that I don't need to repeat this every time I submit a patch ? The patch I made for clang-tools-extra got in with the right name/email pair just fine without me having to say anything, so I was assuming it was there somewhere...)

Thank you! I've gone ahead and landed the changes. I don't think there's a way to add the information to Phab in a way that all the code reviewers will notice. Some reviewers will land based on whatever email address you are using for Phabricator, others ask explicitly, some others will go look at prior reviews to see if they can find the info without asking, etc. FWIW, after you have a few patched submitted on your behalf, we'll often ask if you'd like to obtain commit privileges of your own (https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access).

Thanks for chasing these down!

clang/include/clang/Basic/SourceManager.h
1922

Even though that line was touched, "in-memorty" seems to have managed to stay under the radar. One for the next round. ;-)