[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/test/tools/llvm-elfabi/keep-timestamp.test:1
+# RUN: llvm-elfabi --elf %p/Inputs/gnu_hash.so --emit-tbe=%t --keep-timestamp
+# RUN: touch -m -d "1970-01-01 00:00:00" %t
----------------
You don't need `--keep-timestamp` for the first call, I believe?


================
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"));
 
----------------
`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.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:67
 static Error writeTBE(StringRef FilePath, ELFStub &Stub) {
   std::error_code SysErr;
+  // Write TBE to memory first.
----------------
This is declared too far from the place where it is used.


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:85
+        return Error::success();
+      }
+    }
----------------
You can move/rewrite the comment above and remove curly bracers around this single code line.


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


================
Comment at: llvm/tools/llvm-elfabi/llvm-elfabi.cpp:89
+  // Open TBE file for writing.
+  raw_fd_ostream Out(FilePath, SysErr);
+  Out << TBEStr;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92902