This patch brings various error messages into line with each other, by removing trailing full stops, and making the first letter lower-case. This addresses https://bugs.llvm.org/show_bug.cgi?id=40859.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
- Build Status
Buildable 32292 Build 32291: arc lint + arc unit
Event Timeline
Thanks for the patch @abrachet! I have some comments that you may find useful both here and in the future, regarding the title/description:
In general, title the issue the same as you would the actual commit summary. It's also often useful to put a tag in front representing the tool/library that is being modified (this helps people with searches/email filters etc). Also, put in the description what would go in the commit body (excluding extra bits like the "Differential Revision" and reviewers list. Also, bugs tend to be referred to as PR123456 or simply by their URL in patches. Here's what I'd recommend:
Title: [llvm-objcopy]Tidy up error messages
Description: This patch brings various error messages into line with each other, by removing trailing full stops, and making the first letter lower-case. This addresses https://bugs.llvm.org/show_bug.cgi?id=40859.
It's also useful to put the link to the review request in the bug comments, so that anybody watching the bug can review the patch.
I'm expecting to see a number of test changes. Please make sure to run the lit tests contained within llvm/test/tools/llvm-objcopy with your changes built. This will likely show a number of test failures where we check the error message string, and you'll need to update those too.
llvm/tools/llvm-objcopy/COFF/Object.cpp | ||
---|---|---|
56 | I'm not sure this needs to be quoted, as it is a number, rather than a string. | |
llvm/tools/llvm-objcopy/COFF/Reader.cpp | ||
186 | I think SymbolTableIndex is a variable name, based on the COFF file format, and so should match the actual name. It might be worth confirming this, and if it is, you shouldn't change the case here. If it isn't a part of the file format spec, then it should be something like "symbol table index". | |
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
171 | Remove the full stop here. | |
288–289 | You need to run clang-format here to format this to the LLVM style. | |
llvm/tools/llvm-objcopy/ELF/Object.cpp | ||
1263–1264 | I don't think you need to quote numeric values. The main reason for quoting section/symbol names is because they theoretically can contain spaces. | |
1628–1629 | There's still a trailing full stop here. |
Updated tests, unified the use of 'can't' in error message to the more common 'cannot'
Thanks for the comments @jhenderson!
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
---|---|---|
255 | clang-format did this but I think the previous one looks better |
LGTM, aside from the inline comments. I'd like @jakehehrlich to take a look, given he's the code owner and has final say on these things, but if he doesn't get around to it, I'm happy to commit it tomorrow.
With regards to clang-formatting, make sure to keep the changes to only areas you've touched, as this makes git blame etc harder otherwise (see also the Developer Policy).
llvm/test/tools/llvm-objcopy/ELF/strip-group-symbol.test | ||
---|---|---|
31 | Not related to your change, but you can tidy it up whilst you're here. I think most people find it easier to read the lines with a space between '#' and 'CHECK', so please add it. Same applies to strip-reloc-symbol.test. | |
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
189–191 | If this isn't something you've touched directly, don't change the formatting, even if it's wrong (small changes like this are okay in separate commits though if you're working in the general area). | |
255 | I'd revert it, since this isn't related to your change (see above comment). | |
538–542 | See above, revert this. |
Oh, to make landing this easier for somebody else, please update the description and title as I described in the earlier comment.
This is a super-nit, but the title should have a space, i.e. [llvm-objcopy] Tidy ...
LGTM modulo that and the clang-format thing James mentioned
llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp | ||
---|---|---|
255 | Specifically, git-clang-format handles this well. Editors can also usually be configured for this, depending on which one you use. |
Thanks for fixing all of my mistakes!
I have the same nit about formatting but perhaps that just means we should clang-format the whole thing in a single patch as an NFC. We've done that in the past and I think doing it again would be good.
@abrachet, please make the requested updates and then one of us can commit it for you. Once your first patch lands, you should be able to request commit access as described in the documentation.
Thanks so much for all the comments! Hopefully future patches will be smoother from me. Learned a lot about the process!
Not related to your change, but you can tidy it up whilst you're here. I think most people find it easier to read the lines with a space between '#' and 'CHECK', so please add it. Same applies to strip-reloc-symbol.test.