<div dir="ltr">This looks fine. Did you test outside of llvm? And do you need me to apply?<div><br></div><div>-eric</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jan 9, 2013 at 4:55 PM, 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">Updated patch to address the review comments.<br>
<div class="im">--paulr<br>
<br>
________________________________________<br>
From: <a href="mailto:llvm-commits-bounces@cs.uiuc.edu">llvm-commits-bounces@cs.uiuc.edu</a> [<a href="mailto:llvm-commits-bounces@cs.uiuc.edu">llvm-commits-bounces@cs.uiuc.edu</a>] on behalf of Robinson, Paul [<a href="mailto:Paul.Robinson@am.sony.com">Paul.Robinson@am.sony.com</a>]<br>
</div>Sent: Wednesday, January 09, 2013 3:22 PM<br>
To: Eric Christopher<br>
Cc: <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
Subject: Re: [llvm-commits] [PATCH] PR14471: Debug info for static data members (LLVM part)<br>
<div><div class="h5"><br>
Eric Christopher [<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>] wrote:<br>
> Hi Paul,<br>
><br>
> Few comments:<br>
><br>
> +; RUN: llc %s -o %t -filetype=obj -O0<br>
> +; RUN: llvm-dwarfdump %t | FileCheck %s -check-prefix=CHECK1<br>
> +; RUN: llvm-dwarfdump %t | FileCheck %s -check-prefix=CHECK2<br>
> +; PR14471<br>
><br>
> Can we get a comment on the two checks? I see why (or at least think I<br>
> do) you've got them, but it's not necessarily obvious. I think it's<br>
> because it makes it easier for you to check the things you're looking for.<br>
<br>
Checking that some things do exist and other things do not exist in the<br>
same range of lines is inconvenient to express in FileCheck. Easier to<br>
separate these into separate passes. I do have a comment down where the<br>
actual CHECK lines are, but I can put one here too.<br>
And maybe use better names.<br>
<br>
> +<br>
> +; ModuleID = 'debug-info-static-member.cpp'<br>
> +target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"<br>
> +target triple = "x86_64-unknown-linux-gnu"<br>
<br>
> Go ahead and use an -mtriple flag on the llc command and delete these lines.<br>
> If there's no architecture specific need you can try to move it up to the<br>
> next higher directory.<br>
<br>
Right, I was just taking clang -emit-llvm output verbatim and adding<br>
the CHECK stuff. I think it is actually target-independent but I have<br>
no way to verify that.<br>
<br>
> + /// a typedef, a pointer or reference, etc.; or, a data member of a<br>
> + /// class/struct/union.<br>
><br>
> The ';' is awkward.<br>
<br>
The semicolon separates "data member" from the list of simple derived<br>
types, which is correct. Putting a comma there would make "data member"<br>
just one more element in the list of simple derived types, which it isn't.<br>
<br>
I can make the "data member" part be its own sentence but I'd have to<br>
un-abbreviate "etc." because I was trained not to tolerate abbreviations<br>
at the end of a sentence (or fragment, in this case).<br>
<br>
> +/// getOrCreateContextDIE - Get context owner's DIE.<br>
> +DIE *CompileUnit::getOrCreateContextDIE(DIDescriptor Context)<br>
> +{<br>
><br>
> Formatting.<br>
<br>
Yep.<br>
<br>
> +/// createStaticMemberType - Create debugging information entry for a<br>
> +/// C++ static data member.<br>
> +DIType DIBuilder::createStaticMemberType(DIDescriptor Scope, StringRef Name,<br>
> + DIFile File, unsigned LineNumber,<br>
> + DIType Ty, unsigned Flags,<br>
> + llvm::Value *Val) {<br>
> + assert(Flags & DIDescriptor::FlagStaticMember);<br>
> + // TAG_member is encoded in DIDerivedType format.<br>
><br>
> If you're going to create a new function for creating static member types<br>
> why not just or the FlagStaticMember into the existing flags that are<br>
> passed down rather than making sure it already exists?<br>
<br>
Okay, that makes sense. The caller shouldn't have to supply it.<br>
<br>
Should have refreshed patches by the end of the day.<br>
--paulr<br>
<br>
</div></div>_______________________________________________<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></blockquote></div><br></div>