- User Since
- Oct 10 2016, 10:44 AM (118 w, 6 d)
Fri, Jan 18
@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!
Thu, Jan 17
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!
Wed, Jan 16
Partial support of SHT_GROUP without flag
Take into account @rsmith review.
Added rough support for dependencies withing a group.
Mon, Jan 14
@ruiu I'm not that fmailiar with ldd codebase but I'll happily give it a try!
@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 :-)
Fri, Jan 11
LGTM. I would have kept the default behavior of printing to stdout if -o is not specified, but it's not critical.
Thu, Jan 10
@Romain-Geissler-1A sure thing. Can you rebase your patch on master first?
Wed, Jan 9
@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
Tue, Jan 8
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
Mon, Jan 7
Test case added
Sat, Jan 5
Fri, Jan 4
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.
Thu, Jan 3
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.
Wed, Jan 2
restoring previous diff, I accidentally uploaded an incorrect one on that review :-/
Dec 21 2018
Dec 20 2018
Dec 19 2018
@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.
Dec 18 2018
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.