@rsmith thanks a lot for the final review. I rebased the patch, did a final check with both GCC and Clang, and everything looks fine now. Waiting for a green light!

For some reason, all the specialization were not required, so we end up with a very clean implementation. Thanks a lot @rsmith for pointing that out!

LGTM, thanks

@rsmith up!

Partial support of SHT_GROUP without flag

Take into account @rsmith review.

Added rough support for dependencies withing a group.

@ruiu I'm not that fmailiar with ldd codebase but I'll happily give it a try!

@rsmith: up

@ruiu given you answer to the thread above, I understand we can go on with that review. Anything you're expecting in addition to the current patch?

LGTM, but only from the point of view of python 2/3 compatibility :-)

LGTM. I would have kept the default behavior of printing to stdout if -o is not specified, but it's not critical.

LGTM.

@Romain-Geissler-1A sure thing. Can you rebase your patch on master first?

@bd1976llvm interesting. Unfortunately I failed to find this thread :-/

As a follow up to this review (and the original bug), a thread started on the binutils mlist: https://www.sourceware.org/ml/binutils/2019-01/msg00046.html

Thanks @psmith for the context. I just found the culprit line in gas assembler: https://github.com/bminor/binutils-gdb/blob/502c64b9ac12cf2a35d3cb55c51e2eefd33a2494/bfd/elf.c#L3590

Test case added

LGTM

LGTM, but *only* with respect to the Python 2/3 compatibility of the modification, which is just a drop in the ocean of this patch.

Just wanted to review the change to the Python script to ensure it remains compatible with Python2 and Python3, which is indeed the case; so LGTM on that aspect.

@lebedev.ri : it turns out the only print statement in the two files involved in gbench only have one argument, and they already have the required parethesis. This means that as such, even without the import from future, they work the same in Python2 and Python3. So I will just remove these imports to avoid extra noise.

restoring previous diff, I accidentally uploaded an incorrect one on that review :-/

@chandlerc: thanks for the final review! everything should be okay right now, waiting for @rsmith for the final call.

@michaelplatings I've split this patch in two so that it's easier to review, see https://reviews.llvm.org/D55989 for the companion patch.

@Quuxplusone this patch has been rebuilt without NDEBUG, which triggered a few minor changes I have commented above. If @chandlerc is fine with current state, we should be able to merge it. Then move on to the llvm::Optional special case.

Sorry for the noise :-/

@michaelplatings : some of the non trivial changes are just here to make it easier to write the compatible code. e.g.

Commited as https://llvm.org/svn/llvm-project/cfe/trunk@349447

Thanks @michaelplatings for the review o/