Page MenuHomePhabricator

[lld][ELF] Add negative test coverage for unknown "-z" flags.
AbandonedPublic

Authored by kristina on Nov 15 2018, 3:44 PM.

Details

Reviewers
ruiu
jrtc27
espindola
Group Reviewers
lld
Summary

This makes the code cleaner and adds some coverage for invalid flags being passed as -z <unknown flag>. Almost an NFC change but just wanted to confirm.

Diff Detail

Repository
rLLD LLVM Linker

Event Timeline

kristina created this revision.Nov 15 2018, 3:44 PM
kristina updated this revision to Diff 174296.Nov 15 2018, 3:45 PM

Revised, newline at the end of the test file.

No regressions in ELF test suite and test runs just fine:

=> ninja check-lld-elf

[0/1] Running lit suite /q/src/llvm-taint-8.0/tools/lld/test/ELF
Testing Time: 9.32s
  Expected Passes    : 1054
  Unsupported Tests  : 332
ruiu added a comment.Nov 15 2018, 4:45 PM

Thank you for the patch. But honestly I wouldn't do this personally because the former code only depends on the usual equality check while the new code uses SmallDenseSet which is undeniably (slightly) more complicated than the comparison. From the perspective of code readability, I don't think there's too much difference, it's perhaps a matter of taste. So I think I prefer code that is simple, which is perhaps too simple that it sometimes even looks a bit stupid.

Shall I fully abandon the revision or want me to leave the test for unknown flag failure in? (Unless one already exists and I missed it, additional coverage can't hurt regardless)

kristina updated this revision to Diff 174305.Nov 15 2018, 5:04 PM
kristina retitled this revision from [lld][ELF] Use SmallDenseSet::count instead of lots of logical "or" operators and add test coverage for "-z" flags. to [lld][ELF] Add negative test coverage for unknown "-z" flags..

Revised to drop the change.

ruiu added a comment.Nov 15 2018, 5:21 PM

We do have a test for an unknown -z value. See test/ELF/driver.test.

kristina abandoned this revision.Nov 15 2018, 5:24 PM

Ah alright no problem, seems I didn't notice. Thanks for the quick review.

joerg added a subscriber: joerg.Nov 16 2018, 6:31 AM

Well, I do like the general idea. I would go with a sorted static array and a binary search though. This is not really time critical, but the array version would be smaller in terms of code without involving run-time allocations.