[PATCH] D67758: [llvm-lipo] Add support for archives

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd added inline comments.


================
Comment at: tools/llvm-lipo/llvm-lipo.cpp:217
+    // Replicate the behavior of cctools lipo.
+    Alignment = FO->is64Bit() ? 3 : 2;
+  }
----------------
Can we use the `calculateAlignment` here rather than hardcoding this?  Also, I know its not part of your change, but, could you name `Alginment` as `P2Alignment` to make it clear that this is a power-of-two alignment and not byte alignment.


================
Comment at: tools/llvm-lipo/llvm-lipo.cpp:220
+
+  void setAlignment(uint32_t Align) { Alignment = Align; }
+
----------------
Please name this `setP2Alignment`.


================
Comment at: tools/llvm-lipo/llvm-lipo.cpp:228
+
+  uint32_t getAlignment() const { return Alignment; }
+
----------------
Can we just make `CPUType`, `CPUSubType, `Alignment` `const` members and make them public?  We can set them properly during the constructor and avoid the unnecessary accessors.


================
Comment at: tools/llvm-lipo/llvm-lipo.cpp:246
+    // force arm64-family to follow after all other slices for
+    // compatibility$ with cctools lipo$
+    if (Lhs.CPUType == MachO::CPU_TYPE_ARM64)
----------------
There are a couple of `$` introduced in the comment.


================
Comment at: tools/llvm-lipo/llvm-lipo.cpp:251
+      return true;
+    // Sort by alignment to minimize file size$
+    return Lhs.Alignment < Rhs.Alignment;
----------------
Trailing `$` in the comment?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67758