[PATCH] D92902: [llvm-elfabi] Add flag to keep timestamp when output is the same

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/include/llvm/InterfaceStub/ELFObjHandler.h:37-38
 /// @param Stub Source ELFStub to generate a binary ELF stub from.
 /// @param OutputFormat Target ELFType to write binary as.
 Error writeBinaryStub(StringRef FilePath, const ELFStub &Stub,
+                      ELFTarget OutputFormat, bool PreserveDates = false);
----------------
The comment also needs to be updated.

```
/// @param PreserveDates  .....
```


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:670
+  // Write Stub to memory first.
+  std::unique_ptr<uint8_t> Buf(new uint8_t[Builder.getSize()]);
+  memset(Buf.get(), 0, Builder.getSize());
----------------
This is not a right way to use `unique_ptr` with arrays.
You are calling `new[]` manually and so want the `delete[]` to be called.
You need to write `std::unique_ptr<uint8_t[]>` to achieve that.

Though instead of `std::unique_ptr<uint8_t> Buf`, I think you should just use `vector<uint8_t>` .



================
Comment at: llvm/test/tools/llvm-elfabi/preserve-dates-stub.test:5
+# RUN: touch -m -d "1970-01-01 00:00:00" %t
+# RUN: llvm-elfabi %s --output-target=elf64-little %t -p
+# RUN: ls -l %t | FileCheck %s
----------------
Use the full form, it is reads better and matches the file comment.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:85
+      // If TBE file unchanged, abort updating.
+      if (FileReadBuffer->getBuffer().equals(TBEStr))
+        return Error::success();
----------------
`FileReadBuffer->getBuffer() == TBEStr`?


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:86
+      }
+    }
+  }
----------------
haowei wrote:
> grimar wrote:
> > You should always do something with an error. I.e. either handle it or consume.
> > 
> > This will fail when LLVM is compiled with `-DLLVM_ENABLE_ABI_BREAKING_CHECKS=1`.
> > You should report this error I think.
> Do you mean the SysErr or BufOrError? I think BufOrError (llvm::ErrorOr) only store an error code, do I still need to consume it in case it fail? If so what is the correct way to consume the error from llvm::ErrorOr? I saw llvm::consumeError() but it only takes llvm::Error and llvm::ErrorOr()::getError() only returns a std::error_code.
Sorry, I've misread `ErrorOr` as `Expected` I think. This place looks fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92902