[PATCH] D88827: [llvm-objcopy][NFC] Move core implementation of llvm-objcopy into separate library.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/unittests/Object/ObjCopyTest.cpp:49-50
+  int FD;
+  ASSERT_FALSE(sys::fs::createTemporaryFile("a", "out", FD, Path));
+  FileRemover Cleanup(Path);
+
----------------
jhenderson wrote:
> I was under the impression that temporary files are supposed to be deleted automatically (I might be wrong), so you wouldn't need the `FileRemover`?
> 
> More generally, the need to create a concrete file in the unit test makes me wonder whether we could enhance the objcopy API to take an object of some kind that could represent data in memory (as an alternative to on-disk). I believe such an object hierarchy already exists within LLVM, although the name escapes me right now.
> I was under the impression that temporary files are supposed to be deleted automatically (I might be wrong), so you wouldn't need the FileRemover?

I do not see the place where automatic removing is implemented. Also, there are examples of FileRemover usages in other unit tests:

https://github.com/llvm/llvm-project/blob/7b6790850968031fe1c098ed6dcc196ddc547ea5/llvm/unittests/Support/MemoryBufferTest.cpp#L105



> More generally, the need to create a concrete file in the unit test makes me wonder whether we could enhance the objcopy API to take an object of some kind that could represent data in memory (as an alternative to on-disk). I believe such an object hierarchy already exists within LLVM, although the name escapes me right now.

I think it would be a good enhancement. I propose to do it in a separate patch though.





Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88827/new/

https://reviews.llvm.org/D88827