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

[Digger Lin via Phabricator via cfe-commits]

DiggerLin added a comment.

> I do see the appeal of a single enum parameter in the meantime. I would suggest it should be called SymtabWritingMode and have values NoSymtab, NormalSymtab, BigArchive64Bit, BigArchive32Bit, and BigArchiveBoth. You could optionally drop one of the BigArchive* values, and treat NormalSymtab as that value for BigArchive (as long as "Normal" is clear as to its meaning to someone familiar with BigArchive). All existing WriteSymtab false values would be replaced with NoSymtab and true with NormalSymtab (except for BigArchive cases).

with you suggestion, if I add wrapper and some function member  for SymtabWritingMode ,  there is no different with my current implement with WriteSymTabType.

class WriteSymTabType {
public:

  enum SymtabWritingMode {
    NoSymtab,  // Not write Symbol table.
    Both,     // Write both 32-bit and 64-bit symbol table. for non_AIX, there is always write  both 32-bit and 64-bit symbol table.
    BigArchiveBit32, // Only write the 32-bit symbol table.
    BigArchive64BitBit64  // Only write the 64-bit symbol table.
  };
  
  WriteSymTabType(bool PrintSym) { Value = PrintSym ? Both : None; }
  void operator=(bool PrintSym) { Value = PrintSym ? Both : None; }
  operator bool() { return Value != None; }
  void setBitMode(SymtabWritingMode BM) { Value = BM; }
  SymtabWritingMode getBitMode() { return Value; }

private:

  SymtabWritingMode Value;

};



================
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:
> > > > > 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 
> Well the BitMode is Big Archive specific, and so isn't actually relevant for any other archive format. I don't think it's clear to a maintainer who is unfamiliar with Big Archive that "BitMode = None" implies 2don't write a symbol table". Really, I think the whole llvm-ar.cpp code could do with some major refactoring to introduce more object orientation, so that the details of different formats can be hidden from each other, but that's outside the scope of this patch (in this specific case, I'd have a `SymtabWriter` class with `NoSymtabWriter`, `BigArchiveSymtabWriter32`, and so on subclasses - these would then have methods that did the appropriate writing of symbol tables).
> 
> I do see the appeal of a single enum parameter in the meantime. I would suggest it should be called `SymtabWritingMode` and have values `NoSymtab`, `NormalSymtab`, `BigArchive64Bit`, `BigArchive32Bit`, and `BigArchiveBoth`. You could optionally drop one of the `BigArchive*` values, and treat `NormalSymtab` as that value for BigArchive (as long as "Normal" is clear as to its meaning to someone familiar with BigArchive). All existing `WriteSymtab` `false` values would be replaced with `NoSymtab` and `true` with `NormalSymtab` (except for BigArchive cases).
> 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.

 I am not sure why you do not like the implicit of the conversion semantics of the new class, can you elaborate the disadvantage in the case ?
  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142660