dblaikie added a comment.
In http://reviews.llvm.org/D19567#414906, @probinson wrote:
> Huh. There are strange interactions here, which makes me even more nervous
> about testing fewer cases.
Generally this sort of thing makes me more interested in testing fewer cases so
we can see/make sure they're properly covering, as you just did by the sounds
of it. It's hard to see if everything's really covered if there's lots of
redundant coverage that adds noise to the test case.
> As it happens, the test as written did not exercise all 3 modified paths.
> Because 'struct S2' had all its members marked with nodebug, none of the
> static-member references caused debug info for the class as a whole to be
> attempted.
Not sure I quite follow. Even without nodebug:
struct foo { static const int x = 3; int y; };
int i = foo::x;
doesn't produce any debug info for 'foo', x, etc. Just for 'i'.
> I needed to add a variable of type S2 (without 'nodebug') in order to
> exercise that path. Once that was done, then the modification to
> CollectRecordFields became necessary.
> Even in an unmodified compiler, "static_const_member" never
> shows up as a DIGlobalVariable, although the other cases all do. So, testing
> only for DIGlobalVariable wouldn't be sufficient to show that it gets
> suppressed by 'nodebug'.
Have you tested cases where the static member is ODR used and/or defined:
struct foo { static const int x = 3; };
const int foo::x; // defined
void sink(void*);
void use() { sink(&foo::x); } // ODR used
I'm guessing that the out of line definition will create the DIGlobalVariable
you're not seeing. But probably through a common codepath you've already
corrected for.
The ODR use probably doesn't cause anything interesting to happen.
> "static_member" shows up unless we have modified both
> EmitGlobalVariable(VarDecl case) and CollectRecordFields, given that the test
> actually tries to emit debug info for S2 as a whole.
I would imagine this could still boil down to: check-not DIGlobalVariable,
check-not DIFlagStaticMember ?
But once the test is smaller/more targeted, checking the extra details of the
member list of a composite type, etc, seems OK.
> So, the genuinely most-minimal did-this-change-do-anything test would need
> only "static_member" and "const_global_int_def" to show that each path had
> some effect. This approach does not fill me with warm fuzzies, or the
> feeling that future changes in this area will not break the intended
> behavior. What do you think?
> --paulr
Repository:
rL LLVM
http://reviews.llvm.org/D19567
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
