[PATCH v2 0/14] [x86] Fix 16-bit addressing modes (PR18220) and implement .code16 (PR8684)

Eric Christopher echristo at gmail.com
Thu Dec 19 16:19:32 PST 2013


Can probably just remove the radar comments from x86-16.s since there's no
comments on them and it's likely just a note in the test case of which
radar corresponded to these instructions.

They're also useless comments so I'll probably just remove them from
x86-32.s anyhow.

-eric

On Thu Dec 19 2013 at 4:11:45 PM, Eric Christopher <echristo at gmail.com>
wrote:

> Couple of meta comments on the patches as I've been going through, these
> will seem like nit picking and they are, however...
>
> 1) Comments should be full sentences including punctuation when possible.
>
> 2) When referencing outside sources it's often best to explain in more
> detail what's going on in the particular section as if the external
> reference had evaporated.
>
> 3) I appreciate the splitting up of the patch into multiple and easily
> reviewable chunks.
>
> 4) Testcases should be part of the diff that adds the functionality (in
> particular the ones adding instructions for 16-bit mode).
>
> 5) In general the patches look pretty good. I've been working through the
> various llvm specific things that you've called out - it's been a while
> since I've worked on any of the assemblers as well :)
>
> -eric
>
>
> On Thu Dec 19 2013 at 3:48:05 PM, Eric Christopher <echristo at gmail.com>
> wrote:
>
>
>
> On Thu Dec 19 2013 at 3:47:00 PM, David Woodhouse <dwmw2 at infradead.org>
> wrote:
>
> On Thu, 2013-12-19 at 23:02 +0000, David Woodhouse wrote:
> > I'm just doing a test build before I push the rebased version to my
> > git tree....
>
> Tested and pushed. I elect not to care about seven test failures due to
> the lack of a ppc64 disassembler...
>
>
> Yeah, that requires a reconfigure unfortunately.
>
> -eric
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131220/15c3a697/attachment.html>


More information about the llvm-commits mailing list