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

[Digger Lin via Phabricator via llvm-commits]

DiggerLin added inline comments.


================
Comment at: llvm/lib/Object/ArchiveWriter.cpp:720-724
+        Expected<std::unique_ptr<SymbolicFile>> SymFileOrErr =
+            getSymbolFile(Buf, Context);
+        if (!SymFileOrErr)
+          return createFileError(M->MemberName, SymFileOrErr.takeError());
+        CurSymFile = std::move(*SymFileOrErr);
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > If I'm not mistaken, you could avoid a lot of this duplicate logic about the SymbolicFile by pulling it out of the isAIXBigArchive code. Something like this outline:
> > > 
> > > ```
> > > for (auto M = NewMembers.begin(); M < NewMembers.end(); ++N) {
> > >   if (NeedSymbols || isAIXBigArchive(Kind)) {
> > >     if (M == NewMembers.begin() {
> > >       CurSymFile = // load symbolic file for first member
> > >     } else {
> > >       CurSymFile = std::move(NextSymFile);
> > >     }
> > >     if (M + 1 != NewMembers.end()) {
> > >       NextSymFile = // load symbolic file for next member
> > >     }
> > > 
> > >     // Do all of loop logic
> > >   }
> > > }
> > > ```
> > > You'll need to handle non-symbolic files somehow, perhaps by just resetting the relevant std::unique_ptr to leave it empty, and then checking whether the pointer is empty before trying to read it.
> > in the  if (M == NewMembers.begin())  , it not only open CurSymFile but also calculate MemHeadPadSize 
> > 
> > in the if ((M + 1) != NewMembers.end()),  it not only open CurSymFile but also calculate NextMemHeadPadSize but also calculate the NextMemHeadPadSize
> > 
> > even if I do as your suggestion, I still need to  if (M == NewMembers.begin())  and if ((M + 1) != NewMembers.end()) for MemHeadPadSize  and NextMemHeadPadSize later
> > 
> > 
> > 
> > > You'll need to handle non-symbolic files somehow, perhaps by just resetting the relevant std::unique_ptr to leave it empty, and then checking whether the pointer is empty before trying to read it.
> >   function getSymbolFile already return nullptr for non-symbolic files.
> > 
> > 
> My complaint is that structurally, this and the above block are largely the same. Yes, one does a bit more than the other, but ultimately, the two are structurally identical.
> 
> You could refactor the code to avoid this structural duplication, without too much difficulty, using my suggested code above. To do the extra work regarding calculate the padding sizes, simply add "`if (isAIXBigArchive(Kind))` clauses:
> ```
> for (auto M = NewMembers.begin(); M < NewMembers.end(); ++M) {
>   if (NeedSymbols || isAIXBigArchive(Kind)) {
>     if (M == NewMembers.begin() {
>       CurSymFile = // load symbolic file for first member
>       if (isAIXBigArchive(Kind)) {
>         // Do stuff with padding, as needed.
>       }
>     } else {
>       CurSymFile = std::move(NextSymFile);
>     }
>     if (M + 1 != NewMembers.end()) {
>       NextSymFile = // load symbolic file for next member
>       if (isAIXBigArchive(Kind)) {
>         // Do stuff with padding, as needed.
>       }
>     }
> 
>     // Do all of loop logic (or do some before the ifs and some after).
>   }
> }
> ```
if I change as your suggestion,  
it only reduce the code of 
  
```
 if (!isAIXBigArchive(Kind)) {
        Expected<std::unique_ptr<SymbolicFile>> SymFileOrErr =
            getSymbolFile(Buf, Context);
        if (!SymFileOrErr)
          return createFileError(M->MemberName, SymFileOrErr.takeError());
        CurSymFile = std::move(*SymFileOrErr);
      }
```

I still need to put variables `NextOffset` and `OffsetToMemData`  under the  `if (NeedSymbols || isAIXBigArchive(Kind)) `


and it will several 

`if (isAIXBigArchive(Kind)) ` under  `if (NeedSymbols || isAIXBigArchive(Kind)) {`


we will have duplication code of 
`printMemberHeader(Out, Pos, StringTable, MemberNames, Kind, Thin, *M, ModTime, Size);`


```
  if (NeedSymbols || isAIXBigArchive(Kind)) {
    ...
   if(!isAIXBigArchive(Kind)) 
     printMemberHeader(Out, Pos, StringTable, MemberNames, Kind, Thin, *M, ModTime, Size);
 }
  else {
      printMemberHeader(Out, Pos, StringTable, MemberNames, Kind, Thin, *M, ModTime, Size);
   }  
```

I do not think the logic of your suggestion is clearer than the current code. If you strong suggest , I can change as your suggestion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144872