<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jan 3, 2013 at 11:48 AM, Robinson, Paul <span dir="ltr"><<a href="mailto:Paul.Robinson@am.sony.com" target="_blank">Paul.Robinson@am.sony.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">




<div>
<div style="direction:ltr;font-size:10pt;font-family:Tahoma"><div class="im">
<p>> I think this is going to be OK. I've got a couple comments though:</p>
</div><div style="font-size:16px;font-family:Times New Roman">
<div>
<div dir="ltr"><div class="im">
<div>><br>
</div>
<div>
<div>> -                          layout.getFieldOffset(fieldNo), tunit, RecordTy);</div>
<div>> +                          layout.getFieldOffset(fieldNo - 1), tunit, RecordTy);</div>
<div>><br>
</div>
</div>
</div><div><div class="im">
<div>> a) There are a few other places that use fieldNo and since you're not updating them it'd be nice to have some comments.</div>
<div> </div>
</div><div>I see 4 uses total. One increments it, one decrements it, and this reference uses the value; it had to become "fieldNo - 1" because the increment isn't inside the "for" statement anymore.  Those uses seem like normal tracking behavior and don't warrant
 any extra commentary.  If you still want some, let me know.</div>
<div></div></div></div></div></div></div></div></blockquote><div><br></div><div style>If you don't mind yes please. Especially since you've moved the normal tracking behavior out I'd like a comment.</div><div>
 </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="direction:ltr;font-size:10pt;font-family:Tahoma"><div style="font-size:16px;font-family:Times New Roman">
<div><div dir="ltr"><div><div>The 4th use is in the lambda part of the method where AFAICT it will always be zero (and possibly should be "fieldno" not "fieldNo", is that a bug? I know squat about lambdas).</div>
<div class="im">
<div> </div></div></div></div></div></div></div></div></blockquote><div><br></div><div style>It should be fieldno and not fieldNo, I've got a patch I'll commit when svn is back up. Thanks.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div style="direction:ltr;font-size:10pt;font-family:Tahoma"><div style="font-size:16px;font-family:Times New Roman"><div><div dir="ltr"><div><div class="im">
<div>> b) Instead of inlining all the code for CollectRecordStaticVars how about outlining it into a couple of static functions?</div>
<div> </div>
</div><div>Hmm, yeah the body of that loop is basically "if static-member do this; else if field do that;" so factoring out those bits seems reasonable.  (The lambda part, which I didn't touch, probably also could be factored out for similar reasons, but that might
 exceed the scope of what I'm doing with this patch.  Let me know.)</div><div class="im">
<div> </div></div></div></div></div></div></div></div></blockquote><div><br></div><div style>Yes please, thanks.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div style="direction:ltr;font-size:10pt;font-family:Tahoma"><div style="font-size:16px;font-family:Times New Roman"><div><div dir="ltr"><div><div class="im">
<div>> c) Will this handle something like this:</div>
<div>><br>
</div>
<div>> class A {</div>
<div>>   static int a;</div>
<div>> };</div>
<div>><br>
</div>
<div>> A a;</div>
<div>><br>
</div>
<div>> where the variable A::a isn't defined in the file (but likely is somewhere else).</div>
<div> </div>
</div><div>Sure. You'll get a DIE inside the class with DW_AT_external and DW_AT_declaration, and no DW_AT_location; and then a global variable DIE for the global "a" with type A.  llvm-dwarfdump fragment attached.</div>

<div> </div></div></div></div></div></div></div></blockquote><div><br></div><div style>I think that seems reasonable. Thanks for checking.</div><div style><br></div><div style>In general I'm unconvinced of the need for a new "llvm tag" I'd prefer to keep the tags to dwarf tags to make it easier to find things (and we'll rip out the other ones as we can), and pass information back and forth encoded in the metadata in some other way (a flag perhaps if necessary).</div>
<div style><br></div><div style>-eric</div></div><br></div></div>