On Wed, Jun 16, 2010 at 1:46 PM, Daniel Dunbar <daniel at zuster.org> wrote:> - Please split the patch into at least four parts: > (1) Add WinCOFFStreamer, with a stub implementation. > (2) Add WinCOFFObjectWriter, with a stub implementation. > (3) Fill in WinCOFFStreamer. > (4) Fill in WinCOFFObjectWriter.Attached is patch number 4. - Michael Spencer -------------- next part -------------- A non-text attachment was scrubbed... Name: ms-coff-patch-4.patch Type: application/octet-stream Size: 46306 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100719/172a39d5/attachment.obj>
Hi Michael, Looks great to me, with the caveat that I know nothing about COFF. A few comments: - The term symbol is already very overloaded, I would avoid adding a new 'class Symbol'. - I find it annoying to have more versions of write little endian 32-bit data, but I'm not sure what the right solution is. We can clean this up later, perhaps. - There are a couple of instances of: "}\nelse" instead of "} else". Thanks! - Daniel On Mon, Jul 19, 2010 at 3:09 PM, Michael Spencer <bigcheesegs at gmail.com> wrote:> On Wed, Jun 16, 2010 at 1:46 PM, Daniel Dunbar <daniel at zuster.org> wrote: >> - Please split the patch into at least four parts: >> (1) Add WinCOFFStreamer, with a stub implementation. >> (2) Add WinCOFFObjectWriter, with a stub implementation. >> (3) Fill in WinCOFFStreamer. >> (4) Fill in WinCOFFObjectWriter. > > Attached is patch number 4. > > - Michael Spencer >
Would COFFSymbol be better? I'll fix the others and commit, thanks. Sent from my iPhone On Jul 24, 2010, at 12:24 AM, Daniel Dunbar <daniel at zuster.org> wrote:> Hi Michael, > > Looks great to me, with the caveat that I know nothing about COFF. > > A few comments: > - The term symbol is already very overloaded, I would avoid adding a > new 'class Symbol'. > - I find it annoying to have more versions of write little endian > 32-bit data, but I'm not sure what the right solution is. We can clean > this up later, perhaps. > - There are a couple of instances of: "}\nelse" instead of "} else".