[PATCH] D39000: [ELF] - Implement --orphan-handling option.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: test/ELF/linkerscript/orphan-report.s:46-48
+# RUN: not ld.lld --orphan-handling=discard -o %t.out --script %t.script %t.o 2>&1 \
+# RUN:   | FileCheck %s --check-prefix=UNKNOWN
+# UNKNOWN: unknown --orphan-handling mode: discard
----------------
ruiu wrote:
> grimar wrote:
> > ruiu wrote:
> > > In general, when you give a bogus value, you should give a value that is obviously bogus, because it conveys your meaning better. Instead of "discard", pass "foo", for example. This comment applies to other patches.
> > I passed "discard" here intentionally to document that we do not support this mode explicitly.
> > I can change to "foo" case as well, but I supposed "discard" case is useful itself.
> I don't think I agree. Testing that something is not supported seems a bit odd... Unit tests are to make sure that all your existing features are still working fine, but this test doesn't work for that purpose. We can, for example, add tests to make sure that some Solaris linker options are not supported in lld, but do we want to add such tests? I think the answer is no, because, it doesn't make sense as tests.
May be you are right. I have no strong opinion it seems, both positions seems reasonable for me. 
My concern is that there is some difference between "feature is not supported" and "feature explicitly not supported".
If we choose the second then I think we take responsibility that it should not have unexpected behavior, like
we probably do not want "discard" be just ignored by mistake, what can require test.

I'll change to "foo" before commit though as I have no strong opinion on that point.


https://reviews.llvm.org/D39000