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

[James Henderson via Phabricator via llvm-commits]

jhenderson 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);
+
----------------
avl wrote:
> jhenderson wrote:
> > avl wrote:
> > > 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.
> > > 
> > > 
> > > 
> > > I do not see the place where automatic removing is implemented
> > 
> > For Windows, see references to the OF_Delete flag in Path.inc. For Linux, the behaviour is configured higher up the stack. See `RemoveFileOnSignal` referenced by `TempFile::create`.
> > 
> > At the location you cited, `FileRemover` is needed, because we're not working with a `TempFile` there.
> > 
> > > I think it would be a good enhancement. I propose to do it in a separate patch though.
> > 
> > Agreed, but I think it would be useful to see it before this patch gets committed, to avoid a bad interface landing.
> Ah, the suggestion is to use sys::fs::TempFile::create instead of sys::fs::createTemporaryFile(). I see, thanks.
I didn't realise the two did different things, but it would make sense to do as you are saying, yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88827