[PATCH] D12545: [elf2] Add basic archive file support.

[Rafael Espíndola via llvm-commits]

A few more nits:

+  if (Existing->isDefined() || isa<Lazy>(Existing))
+    return;
+  Sym->Body = New;
+  if (Existing->isUndefined())
+    addMemberFile(New);

The second if should be an assert. On ELF we have

  bool isDefined() const { return !isUndefined(); }

+  // getMember returns an empty buffer if the member was already
+  // read from the library.
+  if (!File)
+    return;

Returns nullptr, no?


+static std::unique_ptr<InputFile> createFile(MemoryBufferRef MB) {
+  // FIXME: duplicated

Please fix it :-)
We need a basic function that only handles objects. It is used in here
and Driver.cpp uses it if not given an archive or shared library.

So only nits. LGTM with all of them fixed.

Cheers,
Rafael




On 3 September 2015 at 18:07, Rafael Espíndola
<rafael.espindola at gmail.com> wrote:
> +  auto ItOrErr = Sym->getMember();
>
> Non obvious type. Please don't use auto.
>
> Can you extend the test to cover us not adding a member twice? Should
> be as simple as adding a second undefined symbol defined by the same
> member.
>
> +  llvm::DenseMap<uint64_t, bool> Seen;
>
> Can this be a DenseSet?
>
> +// Returns a buffer pointing to a member file containing a given symbol.
> +MemoryBufferRef ArchiveFile::getMember(const Archive::Symbol *Sym) {
>
> The comment is effectively duplicated from the header.
>
>
>
> On 3 September 2015 at 17:57, Rafael Espíndola
> <rafael.espindola at gmail.com> wrote:
>> +    case SymbolBody::LazyKind:
>> +      assert("Lazy symbols shouldn't get here.");
>>
>> You want an llvm_unreachable, no? That assert never fails.
>>
>> On 3 September 2015 at 17:48, Michael Spencer <bigcheesegs at gmail.com> wrote:
>>> Bigcheese updated this revision to Diff 33976.
>>> Bigcheese added a comment.
>>>
>>> Address comments.
>>>
>>>
>>> http://reviews.llvm.org/D12545
>>>
>>> Files:
>>>   ELF/Driver.cpp
>>>   ELF/InputFiles.cpp
>>>   ELF/InputFiles.h
>>>   ELF/SymbolTable.cpp
>>>   ELF/SymbolTable.h
>>>   ELF/Symbols.cpp
>>>   ELF/Symbols.h
>>>   ELF/Writer.cpp
>>>   test/elf2/archive.s
>>>