- User Since
- Jul 22 2016, 3:32 PM (151 w, 2 d)
i think we can commit this
Tue, Jun 11
Mon, Jun 10
I've added one inline comment, other than that - LGTM, I'd wait for @mtrent as well )
ok, this makes sense to me now, thanks for the explanation. LGTM with a couple of inline comments
Fri, Jun 7
Thu, Jun 6
khm, could you please explain the rationale for this change ? If yaml2obj completely ignores virtual sections we won't be able to create a MachO binary containing them, moreover, even in some simple cases the round trip conversion yaml -> obj -> yaml will create a completely different binary. Having ability to create binaries with virtual sections overall seems to be useful for testing various tools etc.
the static html file (attached to my previous comments) contains some relative paths to various .css style sheets, so it needs to be placed into <your_build_dir>/docs/html/CommandGuide/ to be rendered correctly
Address @mtrent comments,
I think this looks okay / relatively small change anyway, i don't want to block it anymore, if there are any extra comments I think we can always either revert the diff or address them post-commit.
Wed, Jun 5
@seiya, ok, i think if nobody comments on this diff in the coming days it's safe to proceed and commit it, I kinda don't want to block it and I'm looking forward to more tests in the follow-up patches .
Tue, Jun 4
so in general this looks very promising to me,
however test coverage needs improvements imo, though given that at the moment we don't expose any non-trivial options here, we can add more tests incrementally. Btw - for DYSYMTAB, SYMTAB - is it possible to add YAML-based tests ? or yaml2obj's support is not sufficient for this ?
Mon, Jun 3
Fri, May 31
Thu, May 30
great start! I will take a closer look at the diff tomorrow + over the weekend
Wed, May 29
Tue, May 28
Thu, May 23
May 17 2019
May 16 2019
Use a bigger hammer to parse command line options
May 15 2019
@mtrent, good call, will have a look and update this diff
regarding -verify_arch X Y Z - yeah, I can try to make it work with LLVM's command line options parser, not sure if this interface is ideal / better than -verify_arch X -verify_arch Y -verify_arch Z, but yeah, on the other hand, this would be an incompatibility / would require customers of the old tool to update their scripts/code. The downside of the old interface - the positional argument should go before -verify_arch. To be honest at the moment I don't see how to express the old behavior of -verify_arch using libCommandLine, any suggestions would be helpful, alternatively we can try to live with this incompatibility.
Address comments, have not added docs in this commit yet, can do as a follow-up or will update this diff a bit later.
May 14 2019
will update this diff tomorrow
May 13 2019
- patches which are meant to be sent for code review should include "full context" https://llvm.org/docs/Phabricator.html
- I think it'd be useful to flash out the description of this change (why you are changing this / what does this fix, even if the change is small)
my understanding is that you are trying to change LLVM_TOOLCHAIN_TOOLS - is it correct ?
- I will add one more reviewer to have a look at this diff (just in case).
Apr 25 2019
to me looks good.
Mar 25 2019
Feb 4 2019
Feb 3 2019
Feb 1 2019
Jan 29 2019
Jan 25 2019
I agree with Jordan regarding tests, otherwise this change looks good to me.
Jan 24 2019
Jan 18 2019
i have one inline comment, but otherwise this looks good to me
Jan 17 2019
Jan 10 2019
I'd probably modify the test (to make it work on Windows (if it doesn't)) (or add a comment why it works if it does), other than that (and one minor inline comment) this diff looks like the right fix to me.
Jan 9 2019
on the other hand, i don't really have strong objections against using two dashes if you really want that consistency, not a big deal imo.
tbh i'm not sure about all the cases here (+ not sure if it's really worth doing),
one dash / two dashes - these things are described in tablegen opts files,
i remember there was a bug when 'one dash' was working while 'two dashes' wasn't (i don't already remember the details unfortunately),
so having at least some tests for both is not useless imo.
Jan 7 2019
Jan 4 2019
LGTM with one minor nit
Jan 2 2019
@echristo, oh, i see, thanks.
@lhames , can we proceed here ?
Dec 31 2018
Dec 29 2018
Dec 20 2018
I think this is a step in the right direction, although now we have some inconsistency with the existing code, but okay, I hope we will change the old code in the future as well. Plus I will change the not yet committed code for MachO myself.
Dec 19 2018
Thanks, definitely I'll wait for @lhames