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