[PATCH] D33575: [llvm-ar] Make llvm-lib behave more like the MSVC archiver

[Zachary Turner via llvm-commits]

On Thu, May 25, 2017 at 5:07 PM Reid Kleckner <rnk at google.com> wrote:

> On Thu, May 25, 2017 at 4:46 PM, Rafael Avila de Espindola via
> llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
>>
>> > ===================================================================
>> > --- llvm/lib/Object/COFFImportFile.cpp
>> > +++ llvm/lib/Object/COFFImportFile.cpp
>> > @@ -516,10 +516,9 @@
>> >          OF.createShortImport(*Name, E.Ordinal, ImportType, NameType));
>> >    }
>> >
>> > -  std::pair<StringRef, std::error_code> Result =
>> > -      writeArchive(Path, Members, /*WriteSymtab*/ true,
>> object::Archive::K_GNU,
>> > -                   /*Deterministic*/ true, /*Thin*/ false);
>> > -
>> > +  std::pair<StringRef, std::error_code> Result = writeArchive(
>> > +      Path, Members, /*WriteSymtab=*/true, /*WriteObjPaths=*/true,
>> > +      object::Archive::K_GNU, /*Deterministic=*/true, /*Thin=*/false);
>>
>> This changes WriteObjPaths for COFF import files. Is that intentional?
>>
>
> The names we invent for import files don't have path separators in them,
> so there should be no behavior change either way here. By passing true, we
> don't needlessly run sys::path::filename on them. Are you suggesting that
> maybe it will cause us to not use the short strtab format? I guess I'll
> change it back. Most DLL names are < 16 characters.
>
> > -static bool useStringTable(bool Thin, StringRef Name) {
>> > -  return Thin || Name.size() >= 16;
>> > +static bool useStringTable(bool WriteObjPaths, StringRef Name) {
>> > +  // Force the use of the string table if we're writing full paths.
>> > +  return WriteObjPaths || Name.size() >= 16;
>>
>> I think you want to keep Thin in here. That is a format difference of
>> thin archives:
>>
>> https://sourceware.org/ml/binutils/2008-03/msg00150.html
>
>
> Thin should imply WriteObjPaths. I added an assertion to that effect, but
> I can remove it and thread Thin back here as well if you like.
>

I guess it's possible I'm just missing something, but what I don't
understand is why `Thin` can't just be the only determining factor of
whether we write full paths.  Why the second variable?  AFAICT nowhere in
this patch is `false` ever explicitly passed, it only takes on a `false`
value implicitly when `Thin` is also false.  So just delete the variable,
and only use the value of `Thin`?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170526/2a8ad57d/attachment.html>