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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
