[PATCH] D69041: [yaml2obj][obj2yaml] - Do not create a symbol table by default.

Konrad Wilhelm Kleine via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Oct 17 02:54:04 PDT 2019


kwk added a comment.

In D69041#1712493 <https://reviews.llvm.org/D69041#1712493>, @MaskRay wrote:

> In D69041#1711707 <https://reviews.llvm.org/D69041#1711707>, @kwk wrote:
>
> > @grimar I don't understand why you created this patch, when I'm working on it already.
>
>
> @kwk Sorry if you feel your work is commandeered. On D68943 <https://reviews.llvm.org/D68943>, it was me who said
>
> > Nice! Can you post a differential for the yaml2obj change? I believe the differential can focus on lldb and remove yaml2obj changes (kwk will still get credited for the test cleanups).
>
> I skimmed through your yaml2obj changes in D68943 <https://reviews.llvm.org/D68943> and I believed a complete fix needs more work than your change. `check-llvm-tools check-llvm-object` should expose more tests that you missed.


I know (D68943#1708113 <https://reviews.llvm.org/D68943#1708113>) that I did miss tests and simply haven't updated my diff. I was still fixing some tests locally (D68943#1709330 <https://reviews.llvm.org/D68943#1709330>). I didn't ask nor expected somebody to run tests.

> George has done tons of work in yaml2obj/obj2yaml in the recent months and he probably knows the code better than anyone else. If you read this patch more carefully, you should notice that your version differs a lot from his version.

The basic idea is the same only that he cover's `Link:` entries as well and has fixed more tests (which I also had locally).

> Apologies if you feel sad, but as I said you should still get credits for your lldb changes. Why do you abandon D68943 <https://reviews.llvm.org/D68943>? You can still keep the lldb part.

I will add the LLDB part (which was only about removing `.symtab` manually) once Geroge's test has landed. Let's keep my feelings out of this for a bit ;)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69041/new/

https://reviews.llvm.org/D69041





More information about the llvm-commits mailing list