[PATCH] D63306: Add a remarks-based code size diffing tool

[Francis Visoiu Mistrih via Phabricator via llvm-commits]

thegameg added inline comments.


================
Comment at: llvm/tools/sizediff/CMakeLists.txt:2
+set(LLVM_LINK_COMPONENTS
+  Object
+  Support
----------------
Do we usually sort the components list?


================
Comment at: llvm/tools/sizediff/CMakeLists.txt:12
+if(LLVM_INSTALL_BINUTILS_SYMLINKS)
+  add_llvm_tool_symlink(sizeinfo sizediff)
+endif()
----------------
Any reason why both `sizeinfo` and `sizediff` are not prefixed with `llvm-`?


================
Comment at: llvm/tools/sizediff/sizediff.cpp:91
+    return make_error<StringError>(
+        "Currently, only Mach-O files are supported",
+        std::make_error_code(std::errc::invalid_argument));
----------------
JDevlieghere wrote:
> What happens when you pass a fat binary?
Even more, we should make sure this is a always an object file since the `__LLVM,__remarks` section is not linked in the final binary.


================
Comment at: llvm/tools/sizediff/sizediff.cpp:142
+    int InstrCount;
+    NumInstrs->Val.getAsInteger(0, InstrCount);
+
----------------
I would check for the return value of `getAsInteger` and `continue;` if it fails.


================
Comment at: llvm/tools/sizediff/sizediff.cpp:245
+  // Read in each file. Bail out if an error is encountered at any point.
+  std::string Files[2] = {OldFile, NewFile};
+  for (unsigned Idx = 0; Idx < 2; ++Idx) {
----------------
`StringRef`?


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

https://reviews.llvm.org/D63306