This is an archive of the discontinued LLVM Phabricator instance.

[llvm-objcopy][NFC] Consistenly use two dashes for flags in tests.
ClosedPublic

Authored by rupprecht on Jan 9 2019, 1:27 PM.

Details

Summary

As pointed out in D53667, our use of hyphens in flags can be inconsistent, mixing - with --. This change makes all long style flags use --.

Automatically changed via:

find test/tools/llvm-objcopy/ELF -type f | xargs sed -i 's/ -\([a-zA-Z]\{3\}\)/ --\1/g'

Two false positives were manually fixed/reverted.

Diff Detail

Event Timeline

rupprecht created this revision.Jan 9 2019, 1:27 PM

tbh i'm not sure about all the cases here (+ not sure if it's really worth doing),
one dash / two dashes - these things are described in tablegen opts files,
i remember there was a bug when 'one dash' was working while 'two dashes' wasn't (i don't already remember the details unfortunately),
so having at least some tests for both is not useless imo.

on the other hand, i don't really have strong objections against using two dashes if you really want that consistency, not a big deal imo.

What's most important to me is that we are consistent, primarily within individual files, so that it looks somewhat professional ;-). I do agree that if we want to support single-dash options, we should test both that and double-dash explicitly, much like we test both short and long options. This would probably belong in the "basic" test for each option.

Aside from the fact that LLVM tools traditionally allow it, single-dash long-form arguments seems like a recipe for disaster when mixing in the ability to combine single-letter options into a single argument, and I'm not convinced it should be allowed at all: I haven't tested in detail, but at least for the GNU Win32 version of grep that I have easily to hand, single-dash followed by lots of letters causes issues:

C:\Work>grep -recursive blah .
grep: blah: No such file or directory

C:\Work>grep -recursive "jnjhj" blah
grep: jnjhj: No such file or directory
grep: blah: No such file or directory

C:\Work>grep --recursive "jnjhj" blah
grep: blah: No such file or directory

I don't have objdump or others immediately to hand to try those out.

test/tools/llvm-objcopy/ELF/help-message.test
2

This file in particular should test both single and double dash, because the semantics of single dash are slightly different in general terms (not necessarily in llvm-objcopy, although perhaps they should be), owing to short options being combinable into a single arg.

test/tools/llvm-objcopy/ELF/objcopy-version.test
1 ↗(On Diff #180916)

False one here. This test is actually an example of what I think we should aim to emulate (i.e. short, long with single dash and long with double dash).

test/tools/llvm-objcopy/ELF/strip-version.test
1 ↗(On Diff #180916)

Same as above, don't make this change here.

rupprecht updated this revision to Diff 181181.Jan 10 2019, 4:05 PM
rupprecht marked 3 inline comments as done.
  • Revert some files
  • Add some single dash tests to help-message
This revision is now accepted and ready to land.Jan 11 2019, 1:25 AM
This revision was automatically updated to reflect the committed changes.