[PATCH] D142660: [AIX] supporting -X options for llvm-ranlib in AIX OS

[Digger Lin via Phabricator via cfe-commits]

DiggerLin marked an inline comment as done.
DiggerLin added inline comments.


================
Comment at: llvm/tools/llvm-ar/llvm-ar.cpp:237
 static bool Verbose = false;              ///< 'v' modifier
-static bool Symtab = true;                ///< 's' modifier
+static WriteSymTabType Symtab = true;     ///< 's' modifier
 static bool Deterministic = true;         ///< 'D' and 'U' modifiers
----------------
jhenderson wrote:
> DiggerLin wrote:
> > jhenderson wrote:
> > > DiggerLin wrote:
> > > > jhenderson wrote:
> > > > > Maybe I'm missing something, but I don't see why you need to make this a custom type. You already have the BitMode value that you read in from the environment/command-line, and so you can just use that in conjunction with the `Symtab` boolean to determine what symbol table to write, surely?
> > > > the Symtab are use to specify whether the symbol table need to write(for non-AIX). and what the symbol table need to write for AIX OS.
> > > > 
> > > > there are function like
> > > > 
> > > > ```
> > > >   writeArchiveToBuffer(ArrayRef<NewArchiveMember> NewMembers,
> > > >                      WriteSymTabType WriteSymtab, object::Archive::Kind Kind,
> > > >                      bool Deterministic, bool Thin)
> > > > ```
> > > > and 
> > > > 
> > > > ```
> > > > Error writeArchive(StringRef ArcName, ArrayRef<NewArchiveMember> NewMembers,
> > > >                    WriteSymTabType WriteSymtab, object::Archive::Kind Kind,
> > > >                    bool Deterministic, bool Thin,
> > > >                    std::unique_ptr<MemoryBuffer> OldArchiveBuf = nullptr);
> > > > ```
> > > > 
> > > >  call the function as 
> > > > 
> > > > 
> > > > ```
> > > >         writeArchiveToBuffer(M.second.getMembers(),
> > > >                              /*WriteSymtab=*/true,
> > > >                              /*Kind=*/object::Archive::K_DARWIN,
> > > >                              C.Deterministic,
> > > >                              /*Thin=*/false); 
> > > > ```
> > > > 
> > > > I do not want to change the calling parameter of the /*WriteSymtab=*/true, 
> > > > 
> > > > I introduced a new class WriteSymTabType which has following API
> > > >     
> > > >  
> > > > ```
> > > >  WriteSymTabType(bool PrintSym) { Value = PrintSym ? Both : None; }
> > > >   void operator=(bool PrintSym) { Value = PrintSym ? Both : None; }
> > > >   operator bool() { return Value != None; }
> > > > ```
> > > > 
> > > > 
> > > > 
> > > Having looked at this again, I really don't like the implicit conversion semantics of the new class, and I don't see why you can't just extend the `writeArchiveToBuffer` etc parameters to pass in a `BitMode` parameter.
> > without implicit conversion, if I add a new parameter BitMode to writeArchiveToBuffer,  I also need to add the new parameter BitMode to function writeArchive() and writeArchiveToStream() and there is a lot of place(files) call the function writeArchive, I need to modify all the caller too. 
> > I also need to add the new parameter BitMode to function writeArchive() and writeArchiveToStream() and there is a lot of place(files) call the function writeArchive, I need to modify all the caller too.
> 
> I don't think this is a good reason not to do this, given that it results in an (in my opinion) suboptimal interface. That being said, you could move the new parameter I'm suggesting to the end of the argument list and use a default value, if it helps reduce the impact.
if I add a need BitMode enum
{None,
 Bit32,
 Bit64,
 Bit32_64
};

Do you think we still need the parameter "bool WriteSymtab" , I think we can use the BitMode=None to imply WriteSymtab=false, using  BitMode!=None imply WriteSymtab=true. @jhenderson 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142660