[PATCH] D48413: [llvm-strip] Add initial support for static libraries

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: tools/llvm-objcopy/llvm-objcopy.cpp:481-483
+    if (Error E = writeArchive(Config.OutputFilename, NewArchiveMembers, true,
+                               Ar->kind(), true, Ar->isThin()))
+      reportError(Config.OutputFilename, std::move(E));
----------------
alexshap wrote:
> alexshap wrote:
> > jhenderson wrote:
> > > Test case?
> > again, this Error E can be non-empty if, for example, the syscalls `open` or `write` have failed (+ there might other reasons why writeArchive has returned non-success). 
> > I'm somehow hesitant to test / verify it here - llvm-objcopy is just not the right place for testing writeArchive error-handling. P.S. the same errors might happen when llvm-objcopy or any other tool writes the output to a any file. 
> but since this error is non-recoverable / must not be ignored - we should report it and exit(1) - that's what reportError does.
As noted again, I'm not interested in the cause of the Error, merely the fact that the function returns an Error. In an ideal world, for every conditional branch like an if, we have a test case for it triggering the branch, and a test case for it not triggering the branch, to check and show the behaviour in each instance.

For example, the test case here would check that when an error is reported, a message with the correct output filename is reported. Then, if somebody accidentally changes something in this area that causes the behaviour to change, there is a test to show this.


Repository:
  rL LLVM

https://reviews.llvm.org/D48413