[PATCH] D144872: [AIX] Align the content of an xcoff object file which has auxiliary header in big archive.

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

I'll be honest, I'm not convinced the test coverage looks to be anywhere near comprehensive enough. However, I also haven't attempted to review it versus the code changes to check how much of the new code is actually covered. I'd suggest you consider using a code coverage tool to see how much coverage there is of your new code.



================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:508-511
+// In the AIX big archive format, since the data content follows the member
+// file name, if the name ends on an odd byte, an extra byte will be
+// added for padding. This ensures that the data within the member
+// file starts at an even byte.
----------------
Could you reflow this comment, please? It looks like some of the word wrapping is happening earlier than needed.

A quick look at many of the other new comments in this file makes it look like they have the same issue. Please review all your comments and reflow as necessary.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:544
 
-  return (*ObjOrErr)->is64Bit();
+// AIX Big Archives may contain shared object members. The AIX OS requires
+// these members to be aligned if they are 64-bit and recommends it for
----------------
I can't remember whether we decided it should be "Big Archive" or "big archive". Either way, you should be consistent in all your comments and use one or the other in every case, rather than a mixture (I think "big archive" is the norm elsewhere in this patch).


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:681
 
-  // If the member is non-symbolic file, treat it as having no symbols.
-  if (!*ObjOrErr)
-    return Ret;
+  std::map<std::string, uint16_t> *Map = nullptr;
 
----------------
Could you please move this back to where it was (immediately before the `if (SymMap)`. There's no reason to move it, and by moving it you've left it further from its point of use than it needs to be.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:830-832
+        if (!SymFileOrErr) {
+          return createFileError(MemberName, SymFileOrErr.takeError());
+        }
----------------
No need for the braces here.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:1173
     for (const MemberData &M : Data) {
+      for (uint64_t i = 0; i < M.PreHeadPadSize; i++)
+        Out << '\0';
----------------
It may be slightly more efficient to do something like:
```
OS << std::string(M.PreHeadPadSize, '\0');
```
It's certainly a little more elegant. Alternatively, you could use `std::fill_n`. There are good explanations of both these at https://stackoverflow.com/a/11421689.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144872