<div><font face="arial, sans-serif">Thanks Tim, some great additions here.</font></div><div><font face="arial, sans-serif"><br></font></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>> I've also gone back to the original patch's style of making</span><br style="font-family:arial,sans-serif;font-size:13px"><span style="font-family:arial,sans-serif;font-size:13px">> MCELFStreamer a publicly visible rather than tacking ARMELFStreamer</span><br style="font-family:arial,sans-serif;font-size:13px">

<span style="font-family:arial,sans-serif;font-size:13px">> onto the end of MCELFStreamer.cpp</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">I like that style better too.  I felt tacking ARMELFStreamer onto MCELFStreamer was better for communicating what functionality changed, and planned to move the files in a second patch.  Now that were all on the same page, it makes no difference.  +1</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">> The fragility is my main worry. The patch rather relies on the fact</span><br style="font-family:arial,sans-serif;font-size:13px">

<span style="font-family:arial,sans-serif;font-size:13px">> that EmitBytes and EmitValueImpl are the only functions which will</span><br style="font-family:arial,sans-serif;font-size:13px"><span style="font-family:arial,sans-serif;font-size:13px">> actually emit $d data into a section. Unfortunately, there's no</span><br style="font-family:arial,sans-serif;font-size:13px">

<span style="font-family:arial,sans-serif;font-size:13px">> obvious way to assert on that so tests are the only option.</span><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">True, but I prefer your solution as well.  Using the DataRegion methods was less fragile coming from the IR side, but your solution handles both IR and assembly with the same mechanism.  +1</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><div style="font-family:arial,sans-serif;font-size:13px">

+  virtual void ChangeSection(const MCSection *Section) {</div><div style="font-family:arial,sans-serif;font-size:13px">+    // We have to keep track of the mapping symbol state of any sections we</div><div style="font-family:arial,sans-serif;font-size:13px">

+    // use. Each one should start off as EMS_None, which is provided as the</div><div style="font-family:arial,sans-serif;font-size:13px">+    // default constructor by DenseMap::lookup.</div><div style="font-family:arial,sans-serif;font-size:13px">

+    LastMappingSymbols[getPreviousSection()] = LastEMS;</div><div style="font-family:arial,sans-serif;font-size:13px">+    LastEMS = LastMappingSymbols.lookup(Section);</div><div style="font-family:arial,sans-serif;font-size:13px">

+</div><div style="font-family:arial,sans-serif;font-size:13px">+    MCELFStreamer::ChangeSection(Section);</div><div style="font-family:arial,sans-serif;font-size:13px">+  }</div><div style="font-family:arial,sans-serif;font-size:13px">

<br></div><div><font face="arial, sans-serif">I like it, thank you.  I see that you keep the LastEMS state variable around and therefore only need to do the lookup during ChangeSection() and not in every call to EmitDataMappingSymbol().  </font></div>

<div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif">LGTM!</font></div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">

-Greg</div><div style="font-family:arial,sans-serif;font-size:13px"><br></div></div><div><span style="font-family:arial,sans-serif;font-size:13px"><br></span></div><div class="gmail_extra"><br><br><div class="gmail_quote">

On Mon, Dec 3, 2012 at 8:20 AM, Tim Northover <span dir="ltr"><<a href="mailto:t.p.northover@gmail.com" target="_blank">t.p.northover@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Hi,<br>
<br>
I've done some work on this mapping symbols patch. I think it works<br>
properly for both CodeGen and assembly now, with all the substantial<br>
changes isolated to an ARMELFStreamer.<br>
<br>
I've also gone back to the original patch's style of making<br>
MCELFStreamer a publicly visible rather than tacking ARMELFStreamer<br>
onto the end of MCELFStreamer.cpp. I think this is a neater solution<br>
even in this case, and will get even more so when AArch64 lands and<br>
needs to do something similar.<br>
<div class="im"><br>
> FWIW. It'll still be a whole lots of things to change and a fair bit of fragility<br>
<br>
</div>The fragility is my main worry. The patch rather relies on the fact<br>
that EmitBytes and EmitValueImpl are the only functions which will<br>
actually emit $d data into a section. Unfortunately, there's no<br>
obvious way to assert on that so tests are the only option.<br>
<br>
Ok to commit?<br>
<span class="HOEnZb"><font color="#888888"><br>
Tim.<br>
</font></span></blockquote></div><br></div>