[cfe-commits] [PATCH] PR14471: Debug info for static data members (Clang part)

[Eric Christopher]

On Thu, Jan 3, 2013 at 11:48 AM, Robinson, Paul
<Paul.Robinson at am.sony.com>wrote:

>  > I think this is going to be OK. I've got a couple comments though:
>   >
>  > -                          layout.getFieldOffset(fieldNo), tunit,
> RecordTy);
> > +                          layout.getFieldOffset(fieldNo - 1), tunit,
> RecordTy);
> >
>   > 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.
>
> 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.
>

If you don't mind yes please. Especially since you've moved the normal
tracking behavior out I'd like a comment.


> 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).
>
>

It should be fieldno and not fieldNo, I've got a patch I'll commit when svn
is back up. Thanks.


> > b) Instead of inlining all the code for CollectRecordStaticVars how
> about outlining it into a couple of static functions?
>
> 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.)
>
>

Yes please, thanks.


> > c) Will this handle something like this:
> >
>  > class A {
> >   static int a;
> > };
> >
>  > A a;
> >
>  > where the variable A::a isn't defined in the file (but likely is
> somewhere else).
>
> 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.
>
>

I think that seems reasonable. Thanks for checking.

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).

-eric
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20130103/13f3fe78/attachment.html>