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

[Haowei Wu via Phabricator via llvm-commits]

haowei marked 4 inline comments as done and an inline comment as not done.
haowei added inline comments.


================
Comment at: llvm/test/tools/llvm-elfabi/keep-timestamp.test:4
+# RUN: llvm-elfabi --elf %p/Inputs/gnu_hash.so --emit-tbe=%t --keep-timestamp
+# RUN: stat %t | FileCheck %s
+
----------------
jhenderson wrote:
> Maybe `ls -l` instead? That seems to be what's used in other places. I don't see any usage of `stat` anywhere else.
I changed to 'ls -l' this time. I use the examples in 'llvm/test/tools/llvm-objcopy/ELF/strip-preserve-mtime.test' . 


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:62
+    "keep-timestamp", cl::desc("Keep the timestamp of the output file "
+                               "unchanged if the content is not changed"));
 
----------------
jhenderson wrote:
> grimar wrote:
> > `keep-timestamp` probably sounds like it will always keep the timestamp.
> > I think the name can be better.
> > 
> > Perhaps `write-unchanged`/`update-unchanged` or something else.
> llvm-objcopy uses `-p`/`--preserve-dates`. How about using that?
I think '--preserve-dates' is a better choice.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:86
+      }
+    }
+  }
----------------
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.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:89
+  // Open TBE file for writing.
+  raw_fd_ostream Out(FilePath, SysErr);
+  Out << TBEStr;
----------------
grimar wrote:
> The code on the left uses `SysErr` and might report an error:
> 
> ```
>     return createStringError(SysErr, "Couldn't open `%s` for writing",
>                              FilePath.data());
> ```
> 
> You code ignores it.
Thanks for pointing it out. I found it and corrected it when I commit the code but I forgot to update the diff file in Phabricator. I correct it in the latest diff.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92902