<div><span style="font-family:arial,sans-serif;font-size:13px">Tim, thank you for the review. Comments below:</span><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><span style="font-family:arial,sans-serif;font-size:13px"><div>
<span style="font-family:arial,sans-serif;font-size:13px"><br></span></div>> First, I'd say that I preferred the refactoring of the previous patch. I</span><br style="font-family:arial,sans-serif;font-size:13px"><span style="font-family:arial,sans-serif;font-size:13px">> prefer to keep as much target-specific code as possible in its directory.</span><br>
<div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px">When I add the fixes you have </span><span style="font-family:arial,sans-serif;font-size:13px">suggested, I will also include a second</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">patch that does nothing </span><span style="font-family:arial,sans-serif;font-size:13px">but the file refactoring.</span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br>
</span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px">> I'm afraid I disagree with ignoring ARM/Thumb switchover too.</span><span style="font-family:arial,sans-serif;font-size:13px"><br>
</span></div><div><br></div><div><font face="arial, sans-serif">I don't disagree with your disagreement. :) I am hoping to prioritize fixing</font></div><div><font face="arial, sans-serif">correctness bugs </font><font face="arial, sans-serif">in the assembler. But s</font><span style="font-family:arial,sans-serif">ince you have provided clear</span></div>
<div><span style="font-family:arial,sans-serif">direction on implementation, I'd be happy to give it a shot.</span></div><div><span style="font-family:arial,sans-serif"><br></span></div><div><span style="font-family:arial,sans-serif"><br>
</span></div><div><span style="font-family:arial,sans-serif;font-size:13px">> You've also forgotten the tests for STT_NOTYPE and STB_LOCAL</span><span style="font-family:arial,sans-serif"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br>
</span></div><div><font face="arial, sans-serif">I see. Will fix!</font></div><div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif"><br></font></div><div><span style="font-family:arial,sans-serif;font-size:13px">> which tests </span><span style="font-family:arial,sans-serif;font-size:13px">are the ones that check the duplication code?</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><font face="arial, sans-serif">There is no test for checking for duplicates. I don't know how to do that</font></div><div><font face="arial, sans-serif">with FileCheck. Is there a way or would I need another tool?</font></div>
<div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif"><br></font></div><div><div><span style="font-family:arial,sans-serif;font-size:13px">> Wouldn't you </span><span style="font-family:arial,sans-serif;font-size:13px">need two functions </span><span style="font-family:arial,sans-serif;font-size:13px">to force the Streamer to </span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">> ChangeSection to .text twice?</span></div></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px">I don't understand. Can you clarify?</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div><span style="font-family:arial,sans-serif;font-size:13px">Thanks,</span></div>
<div><span style="font-family:arial,sans-serif;font-size:13px">Greg</span></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Nov 13, 2012 at 2:47 AM, Tim Northover <span dir="ltr"><<a href="mailto:Tim.Northover@arm.com" target="_blank">Tim.Northover@arm.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Greg,<br>
<div class="im"><br>
On Friday 09 Nov 2012 22:14:53 Greg Fitzgerald wrote:<br>
> Compared to the previous patch, this one is small, simple and comes with a<br>
> unit-test. It doesn’t move any files around and does its best to be a<br>
> good citizen with regard to coding style. Could you please review?<br>
<br>
</div>First, I'd say that I preferred the refactoring of the previous patch. I<br>
prefer to keep as much target-specific code as possible in its directory.<br>
<br>
I'm afraid I disagree with ignoring ARM/Thumb switchover too. As Jan points<br>
out, that part of ARM assembly is supported, except that mapping symbols won't<br>
work with it.<br>
<br>
As far as I can tell it would be fairly simple to implement. You just need to<br>
also override the EmitAssemblerFlag function in the ARMELFStreamer and do the<br>
right thing for MCAF_Code16 and MCAF_Code32.<br>
<br>
You've also forgotten the tests for STT_NOTYPE and STB_LOCAL, and which tests<br>
are the ones that check the duplication code? Wouldn't you need two functions<br>
to force the Streamer to ChangeSection to .text twice?<br>
<br>
Other than that, the patch seems reasonable to me. The symbols have the<br>
correct type and it's valid to omit them for data-only sections so I think the<br>
ABI is satisfied.<br>
<br>
Regards.<br>
<span class="HOEnZb"><font color="#888888"><br>
Tim.<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>