This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy] Tidy up error messages
ClosedPublic

Authored by abrachet on May 17 2019, 11:20 AM.

Event Timeline

abrachet created this revision.May 17 2019, 11:20 AM

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.

abrachet updated this revision to Diff 200358.May 20 2019, 2:27 PM

Updated tests, unified the use of 'can't' in error message to the more common 'cannot'

abrachet marked 6 inline comments as done.May 20 2019, 2:35 PM

Thanks for the comments @jhenderson!

llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp
254

clang-format did this but I think the previous one looks better

abrachet marked an inline comment as done.May 20 2019, 2:35 PM
jhenderson accepted this revision.May 21 2019, 2:16 AM

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–190

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).

254

I'd revert it, since this isn't related to your change (see above comment).

538–540

See above, revert this.

This revision is now accepted and ready to land.May 21 2019, 2:16 AM

Oh, to make landing this easier for somebody else, please update the description and title as I described in the earlier comment.

abrachet retitled this revision from Worked on bug 40859 to be more consistent with error messages in llvm-objcopy to [llvm-objcopy]Tidy up error messages.May 21 2019, 5:06 PM
abrachet edited the summary of this revision. (Show Details)
rupprecht accepted this revision.May 21 2019, 5:36 PM

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
254

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.

jakehehrlich accepted this revision.May 21 2019, 8:16 PM

@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.

abrachet updated this revision to Diff 200692.May 22 2019, 3:47 AM
abrachet edited the summary of this revision. (Show Details)

Reverted a change made by clang-format

abrachet retitled this revision from [llvm-objcopy]Tidy up error messages to [llvm-objcopy] Tidy up error messages.May 22 2019, 3:47 AM
abrachet marked an inline comment as done.May 22 2019, 3:52 AM

Thanks so much for all the comments! Hopefully future patches will be smoother from me. Learned a lot about the process!

abrachet updated this revision to Diff 200694.May 22 2019, 3:55 AM

Missed a couple changes that clang-format made

abrachet updated this revision to Diff 200697.May 22 2019, 4:09 AM
abrachet marked 3 inline comments as done.

Tidied up test files to make more readable. Put a space between '#CHECK'

abrachet marked an inline comment as done.May 22 2019, 4:09 AM
jhenderson accepted this revision.May 22 2019, 6:02 AM

LGTM! Thanks. I'll try to commit this later today.

This revision was automatically updated to reflect the committed changes.