I have updated my patch based on Chris'es feedback. I removed the dbgout_calls macro, but left others in place for now. If there are no objections, I would like to commit this tomorrow evening (~7PM GMT-7). I have compiled and tested it on MSVC, with Michaels testing code and it looks good. Once this is committed, Michaels patch can be applied. -Nathan -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100615/f4b28bed/attachment.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: changes.patch Type: application/octet-stream Size: 47596 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100615/f4b28bed/attachment.obj>
Hi Nathan,
I'm not quite ready to apply this patch, but I now have time again to
work on getting it in.
The main reason I am not willing to put it in yet is (a) the size, and
(b) the inconsistences with LLVM style. Please fix the following
things:
- Watch out for case mismatches, i.e., #include "WinCoff.h" when the
header is actually WinCOFF.h.
- The patch doesn't have a consistent newline convention, it is a
mix of DOS and Unix.
- if( -> if (
- A [1] -> A[1]
- Braces go on the same line as if/else.
- Please use AddFragment instead of add_fragment, etc, and we tend
to only use a leading lower case for accessor methods (getFoo) or
trivial inlines functions, but not for methods which do work or have
substantial side effects.
- Please use complete sentences in comments, i.e. capitalized and
ending with proper punctuation.
- Please don't use an extra newline following 'for' loops,
conditionals, or type declarations.
- Please see http://llvm.cs.uiuc.edu/docs/CodingStandards.html if
you haven't already.
There are also some meatier things I would like changed:
- 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.
The first two parts should be trivial, and as small as possible. It
is useful to get them in first to make subsequent patches easier to
read.
- Please put the COFF constants which are in WinCOFF.h in
include/llvm/Support/COFF.h.
o This should only be the stuff which pertains to the file format,
and is not implementation dependent (i.e., no structure definitions
unless they exactly match the file format, and are pure C structs).
- Please move the remaining content in WinCOFF.h into
WinCOFFObjectWriter.cpp, since it is implementation dependent and
shouldn't need to be exposed.
- I have some more comments about the actual implementation, but I'd
like to see smaller patches before getting in to details.
- Daniel
On Tue, Jun 15, 2010 at 12:47 AM, Nathan Jeffords
<blunted2night at gmail.com> wrote:> I have updated my patch based on Chris'es feedback. I removed the
> dbgout_calls macro, but left others in place for now. If there are no
> objections, I would like to commit this tomorrow evening (~7PM GMT-7). I
> have compiled and tested it on MSVC, with Michaels testing code and it
looks
> good. Once this is committed, Michaels patch can be applied.
> -Nathan
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
On Wed, Jun 16, 2010 at 1:46 PM, Daniel Dunbar <daniel at zuster.org> wrote:> The main reason I am not willing to put it in yet is (a) the size, and > (b) the inconsistences with LLVM style. Please fix the following > things: > - Watch out for case mismatches, i.e., #include "WinCoff.h" when the > header is actually WinCOFF.h. > - The patch doesn't have a consistent newline convention, it is a > mix of DOS and Unix. > - if( -> if ( > - A [1] -> A[1] > - Braces go on the same line as if/else. > - Please use AddFragment instead of add_fragment, etc, and we tend > to only use a leading lower case for accessor methods (getFoo) or > trivial inlines functions, but not for methods which do work or have > substantial side effects. > - Please use complete sentences in comments, i.e. capitalized and > ending with proper punctuation. > - Please don't use an extra newline following 'for' loops, > conditionals, or type declarations. > - Please see http://llvm.cs.uiuc.edu/docs/CodingStandards.html if > you haven't already. > > - DanielNathan and I decided to work on this patch on github until it's finally ready and submit the proper patches against svn HEAD then. I would like someone to review this specific diff purely for the LLVM style issues that Daniel noted and let me know if there are any outstanding issues on that end. http://github.com/Bigcheese/llvm-mirror/commit/76b76b90cebba28bdfa6c5ec0578b887c6b91f79 - Michael Spencer
On Mon, Jun 21, 2010 at 9:37 AM, Aaron Gray <aaronngray.lists at gmail.com> wrote:> > Whats going on with having both write_uint32_le() > in WinCOFFObjectWriter::DefineSection() and WriteLE32() in > WinCOFFObjectWriter::WriteFileHeader() ? > AaronThanks, missed that one. Nathan used write_uint32_le in WinCOFF.h, and I guess accidentally used it in WinCOFFObjectWriter.cpp. I'll remove the calls from WinCOFFObjectWriter.cpp now, and remove them (and the decl+deff) from WinCOFF.h when it gets moved over to Support/COFF.h. - Michael Spencer
write_uint32_le is used to write into a arbitrary data buffer, where as WriteLE32 is used to write into the object file. On Mon, Jun 21, 2010 at 9:01 AM, Michael Spencer <bigcheesegs at gmail.com>wrote:> On Mon, Jun 21, 2010 at 9:37 AM, Aaron Gray <aaronngray.lists at gmail.com> > wrote: > > > > Whats going on with having both write_uint32_le() > > in WinCOFFObjectWriter::DefineSection() and WriteLE32() in > > WinCOFFObjectWriter::WriteFileHeader() ? > > Aaron > > Thanks, missed that one. Nathan used write_uint32_le in WinCOFF.h, and > I guess accidentally used it in WinCOFFObjectWriter.cpp. I'll remove > the calls from WinCOFFObjectWriter.cpp now, and remove them (and the > decl+deff) from WinCOFF.h when it gets moved over to Support/COFF.h. > > - Michael Spencer > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100621/bf003066/attachment.html>