<div>Michael,</div><div><br></div>I have not made any changes since I last posted a patch. If you would like to make the final updates, thats fine by me. I don't mind making the changes either, I can have them done this evening as well.<div>
<br></div><div>Chris,</div><div><br></div><div>As far as naming goes, PE/COFF used in Microsoft's document because they are describing both their version of COFF, and their Portable Execution format which is a slightly modified version of their COFF format. I don't think that PE belongs there since this code doesn't generate executables. I used the Win prefix to differentiate it from non Microsoft Windows versions of COFF.</div>
<div><div><br></div><div>For the debug macros, I personally like them because they reduce the impact debug code has on the "real" code. I find dbgout("message") simpler and more concise than DEBUG(dbgs() << "message") for the consumer of the debug API. The other macros are there to not have to reproduce the same pattern a repeatedly.</div>
<div><br></div><div>-Nathan</div><br><div class="gmail_quote">On Mon, Jun 14, 2010 at 12:25 PM, Michael Spencer <span dir="ltr"><<a href="mailto:bigcheesegs@gmail.com">bigcheesegs@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
Nathan, do you have any other changes? If not, I can make these changes and have a patch ready this evening.<br>
<br>
Sent from my iPhone<div><div></div><div class="h5"><br>
<br>
On Jun 14, 2010, at 2:16 PM, Chris Lattner <<a href="mailto:clattner@apple.com" target="_blank">clattner@apple.com</a>> wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On Jun 11, 2010, at 6:44 PM, Michael Spencer wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Here is a full patch including Nathan's COFF support, tests that pass<br>
on Windows, and some changes to lit.<br>
<br>
Obviously the COFF support and changes to lit should be separate<br>
patches in the end.<br>
<br>
<a href="http://codereview.appspot.com/1624043/show" target="_blank">http://codereview.appspot.com/1624043/show</a><br>
</blockquote>
<br>
Hi Guys,<br>
<br>
What is the status of this patch? Here are some comments on the code, can someone update and send the most recent version?<br>
<br>
I'd like to get this cleaned up applied even if it isn't functionally perfect yet.<br>
<br>
Thanks!<br>
<br>
<br>
A general comment: for many of these, I only list one instance of a particular issue.<br>
<br>
<br>
+++ lib/MC/WinCOFF.h (working copy)<br>
@@ -0,0 +1,327 @@<br>
+//===-- llvm/MC/MCWin32Coff.h -----------------------------------*- C++ -*-===//<br>
<br>
Please fix the filename in the comment line.<br>
<br>
+#include <llvm/MC/MCObjectWriter.h><br>
+#include <llvm/ADT/SmallString.h><br>
+#include <llvm/ADT/SmallVector.h><br>
<br>
Please use "" style includes for llvm headers.<br>
<br>
<br>
<br>
+namespace coff {<br>
+<br>
+ using llvm::raw_ostream;<br>
+ using llvm::MCSymbolData;<br>
+ using llvm::MCSectionData;<br>
<br>
Please don't use 'using' directives in headers.<br>
<br>
<br>
<br>
+ uint8_t * Ptr = reinterpret_cast <uint8_t *> (Data);<br>
+ Ptr [0] = (Value & 0x000000FF) >> 0;<br>
<br>
It would be more consistent to space this sort of thing as:<br>
<br>
+ uint8_t *Ptr = reinterpret_cast<uint8_t *>(Data);<br>
+ Ptr[0] = (Value & 0x000000FF) >> 0;<br>
<br>
<br>
<br>
Notably, we don't put spaces before parens in function calls or before template arguments:<br>
<br>
+ symbol(llvm::StringRef Name, size_t) :<br>
+ Name(Name.begin (), Name.end ()),<br>
...<br>
+ typedef std::vector <symbol*> symbols;<br>
<br>
<br>
<br>
<br>
<br>
+ // this class contians staging data for a coff section<br>
+ struct section {<br>
<br>
typo 'contians'. COFF should be capitalized and the comment should end with a period.<br>
<br>
<br>
+//===-- llvm/MC/WinCOFFObjectWriter.cpp -------------------------*- C++ -*-===//<br>
+//<br>
<br>
A terminology question: is it "Win32 COFF" file or "PECOFF" file? What is the difference? What does Win64 use?<br>
<br>
<br>
<br>
+#include <stdio.h><br>
<br>
Do you really need this? If so, please use <cstdio><br>
<br>
<br>
<br>
+ assert (Section->Symbol->Aux.size () == 15);<br>
+ write_uint32_le(Section->Symbol->Aux.data() + 0, Section->Header.SizeOfRawData);<br>
+ write_uint16_le(Section->Symbol->Aux.data() + 4, Section->Header.NumberOfRelocations);<br>
+ write_uint16_le(Section->Symbol->Aux.data() + 6, Section->Header.PointerToLineNumbers);<br>
...<br>
+ void add_common_symbol(MCSymbol *Symbol, uint64_t Size, unsigned ByteAlignment, bool External);<br>
<br>
Please wrap to 80 columns.<br>
<br>
+ for (coff::relocations::const_iterator k = (*i)->Relocations.begin();<br>
+ k != (*i)->Relocations.end();<br>
+ k++) {<br>
<br>
No need to reevaluate ".end()" every iteration of the loop.<br>
<br>
<br>
+#define dbgout(x) DEBUG(dbgs() << x)<br>
+#define dbgerr(x) do { dbgout (__FILE__ << "(" << __LINE__ <<\<br>
+ ") : " << x << '\n'); llvm_unreachable("unexpected error occured");\<br>
+ } while (false)<br>
+<br>
+#define dbgout_call(x) DEBUG_WITH_TYPE(DEBUG_TYPE"_calls", dbgs() << \<br>
+ __FUNCTION__ << " (" << x << ")\n")<br>
+<br>
+#define dbg_notimpl(x) dbgerr("not imlemented, " << __FUNCTION__ \<br>
+ << " (" << x << ")")<br>
<br>
I'm not a big fan of these macros (plus typos like imlemented) are they really needed?<br>
<br>
Overall, looks like great progress!<br>
<br>
-Chris<br>
</blockquote>
</div></div></blockquote></div><br></div>