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

[Michael Trent via Phabricator via llvm-commits]

mtrent accepted this revision.
mtrent added a comment.

Logic looks fine. I have some concerns around alignment terminology that I suggest you address before committing.



================
Comment at: tools/llvm-lipo/llvm-lipo.cpp:105
+// For compatibility with cctools lipo, alignment is calculated as the minimum
+// aligment of all segments. Each segments's alignment is the maximum alignment
+// from its sections
----------------
A segment alignment has no strong correlation to the section alignment, and the notion that the segment alignment is the maximum alignment of its sections is not true. Maybe the simplest way to say this is:

"For compatibility with cctools lipo, a file's alignment is calculated as the minimum aligment of all segments. For object files, the file's alignment is the maximum alignment of its sections."

Part of the problem is that the method here is named "calculateSegmentAlignment" but that 1) doesn't accurately describe how lipo is using that value, and 2) also doesn't completely describe what the function is doing. But cctools also has both these problems :) You have an opportunity to call this function something like "calculateSliceAlignment" or "calculateFileAlignment" which may be objectively better ... 

Be aware the segment computation is merely a guess, and it may not be exactly correct. But it's correct enough for lipo.

And be aware the behavior for object files may change in future versions of cctools.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D67758