[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:11:45 PST 2013


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/618c0da3/attachment.html>


More information about the llvm-commits mailing list