ping On Mon, May 2, 2016 at 11:51 AM, David Blaikie via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> 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 >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
