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

[Alexander Shaposhnikov via Phabricator via llvm-commits]

alexshap added inline comments.


================
Comment at: test/tools/llvm-objcopy/strip-all.test:27
+# RUN: llvm-strip %t.a
+# RUN: llvm-ar -p %t.a %basename_t.tmp >%t6
+# RUN: cmp %t2 %t6
----------------
jhenderson wrote:
> alexshap wrote:
> > jhenderson wrote:
> > > The "-" part of "-p" is unnecessary so can be removed (either that or you should add it to "crs" for consistency).
> > > 
> > > What is the purpose of "%basename_t.tmp"?
> > > 
> > > Does lit use it's own redirection mechanism? I ask because on Windows Powershell encodes files created via ">" as UTF-16, which obviously isn't suitable for binary files. I'd be more inclined to use the extract operation (x) to achieve the same thing, as it's less likely to cause a problem.
> > %basename_t.tmp is expanded by lit into the basename of %t .
> > so llvm-ar p %t.a %basename_t.tmp prints the content of a given file from the archive.
> > -x will extract a file into the current folder so it will overwrite the existing file, I somehow didn't like this.
> >  
> Yeah, the overwriting isn't great, and I get why you didn't want to use -x in the first place. But does the redirection work for all users? I might have to test this myself, since I use Windows and Powershell as my primary development environment, and it's important that the binary file doesn't get encoded as it gets printed out.
> 
> So we need %basename_t because %t is an absolute path, right? I think this sounds more like a deficiency in the test framework - I don't like the need to know that %t ends up as <some path>.tmp for this test to work, as it should be an implementation detail that the test doesn't care about. That being said, it's probably outside the scope of this change to worry about that, if there is no clear alternative (but perhaps a new macro that expands to <the same as %t without the directory components> might be appropriate at some point).
yeah


Repository:
  rL LLVM

https://reviews.llvm.org/D48413