On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li <davi...@google.com> wrote:
> To be clear, you are suggesting breaking the test into two (one for > copy, and one for move) ? I am totally fine with that. Nah, no need to split the test case - we try to keep the number of test files down (& group related tests into a single file) to reduce test execution time (a non-trivial about of check time is spent in process overhead). > I thought you > suggested removing the testing of move/op case because they might > share the same code path (clang's implementation) as the copy/op. > I was suggesting that two cases is no big deal whether you test both or test one if they're the same codepath - if there were 5/many more things that shared the same codepath, I'd generally suggest testing a representative sample (possibly just a single one) rather than testing every client of the same code. Feel free to leave the two here as-is. (though if we're talking about test granularity, it's probably worth just putting these cases in the same file/type/etc as the ctor cases you mentioned were already covered) & I'm still curious/wondering if there's a common codepath that would provide a simpler fix/code that solved both implicit and explicitly defaulted ops. - Dave > > thanks, > > David > > On Mon, Feb 8, 2016 at 3:52 PM, David Blaikie <dblai...@gmail.com> wrote: > > > > > > On Mon, Feb 8, 2016 at 3:46 PM, Xinliang David Li <davi...@google.com> > > wrote: > >> > >> On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie <dblai...@gmail.com> > wrote: > >> > > >> > > >> > On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li <davi...@google.com > > > >> > wrote: > >> >> > >> >> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie <dblai...@gmail.com> > >> >> wrote: > >> >> > > >> >> > > >> >> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li > >> >> > <davi...@google.com> > >> >> > wrote: > >> >> >> > >> >> >> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie < > dblai...@gmail.com> > >> >> >> wrote: > >> >> >> > > >> >> >> > > >> >> >> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits > >> >> >> > <llvm-comm...@lists.llvm.org> wrote: > >> >> >> >> > >> >> >> >> davidxl updated this revision to Diff 47217. > >> >> >> >> davidxl added a comment. > >> >> >> >> > >> >> >> >> Simplified test case suggested by Vedant. > >> >> >> >> > >> >> >> >> > >> >> >> >> http://reviews.llvm.org/D16947 > >> >> >> >> > >> >> >> >> Files: > >> >> >> >> lib/CodeGen/CGClass.cpp > >> >> >> >> test/Profile/def-assignop.cpp > >> >> >> >> > >> >> >> >> Index: test/Profile/def-assignop.cpp > >> >> >> >> > >> >> >> >> > =================================================================== > >> >> >> >> --- test/Profile/def-assignop.cpp > >> >> >> >> +++ test/Profile/def-assignop.cpp > >> >> >> >> @@ -0,0 +1,32 @@ > >> >> >> >> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple > >> >> >> >> x86_64-unknown-linux-gnu > >> >> >> >> -main-file-name def-assignop.cpp -o - -emit-llvm > >> >> >> >> -fprofile-instrument=clang > >> >> >> >> | FileCheck --check-prefix=PGOGEN %s > >> >> >> >> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple > >> >> >> >> x86_64-unknown-linux-gnu > >> >> >> >> -main-file-name def-assignop.cpp -o - -emit-llvm > >> >> >> >> -fprofile-instrument=clang > >> >> >> >> -fcoverage-mapping | FileCheck --check-prefix=COVMAP %s > >> >> >> >> + > >> >> >> >> +struct B { > >> >> >> >> + void operator=(const B &b) {} > >> >> >> >> + void operator=(const B &&b) {} > >> >> >> > > >> >> >> > > >> >> >> > Probably best to make these canonical to avoid confusion: > >> >> >> > > >> >> >> > B &operator=(const B&); > >> >> >> > B &operator=(B&&); > >> >> >> > > >> >> >> > (& they don't need definitions - just declarations) > >> >> >> > >> >> >> Will change. > >> >> >> > >> >> >> > > >> >> >> > Also, neither of these are the move /constructor/, just the move > >> >> >> > operator. > >> >> >> > Not sure if Vedant just used the wrong terminology, or whether > >> >> >> > it's > >> >> >> > worth > >> >> >> > testing the move/copy ctors too, to check that they do the right > >> >> >> > thing > >> >> >> > as > >> >> >> > >> >> >> I added tests for copy ctors, and plan to add move ctor test soon. > >> >> >> > >> >> >> > well. (if all of these things use the same codepath, I don't > see a > >> >> >> > great > >> >> >> > benefit in having separate tests for them (but you can add them > >> >> >> > here > >> >> >> > if > >> >> >> > you > >> >> >> > like) - I'm just suggesting a manual verification in case those > >> >> >> > need > >> >> >> > a > >> >> >> > separate fix > >> >> >> > >> >> >> the ctor and assignment op do not share the same path -- the ctor > >> >> >> path > >> >> >> is working as expected without the fix -- or do you mean there is > no > >> >> >> need to cover both copy and move variants? > >> >> > > >> >> > > >> >> > I wouldn't necessarily bother testing multiple instances of the > same > >> >> > codepath (so the copy and move ctor for example) - but 2 instances > is > >> >> > no > >> >> > big > >> >> > deal (if there were several more, I might be inclined to just test > >> >> > one > >> >> > as a > >> >> > representative sample). I don't mind either way, though. The number > >> >> > is > >> >> > small > >> >> > & the test cases are arguably distinct. > >> >> > >> >> Sorry I disagree with your general statement here. I treat such test > >> >> cases as 'black box testing' that do not know about the internal > >> >> implementation (code path). It may or may not share the same code > path > >> >> today -- same is true in the future. > >> > > >> > > >> > While there's merit in both approaches, practically speaking it seems > >> > difficult to test in that way in general - any feature could interact > >> > with > >> > any other. > >> > >> The language features are well specified -- so writing small test > >> cases to cover them is a general accepted way of testing. > > > > > > I'm not sure I follow the distinction you're drawing between the middle > end > > optimization tests and the features you're testing here. If the features > are > > relatively independent, even within the same API/feature area, they're > > generally tested independently (even two features within a single middle > end > > optimization - a test case is written to ensure that, say, > ArgumentPromotion > > correctly handles debug info, and another that it correctly handles > inalloca > > (or fp80, etc - just looking at test/Transforms/ArgumentPromotion) - but > we > > don't test the matrix of combinations of these features) > > > >> > >> > >> >The LLVM regression suite is far more narrowly targeted than that > >> > - we don't test combinations of optimizations, for example - we test > >> > each > >> > optimization in isolation. The same would be true of two independent > >> > features on an interface such as this, I think. > >> > >> This is a weakness of the test system -- a problem at a different > >> dimension. > > > > > > If we want to have a discussion about the LLVM community testing > > methodology, that might be best taken up on llvm-dev (and clang-dev). But > > for now, I'd ask that tests in the lit regression suite are generally as > > isolated as possible and test one thing at a time. > > > >> > >> > >> > > >> >> > >> >> > >> >> > > >> >> >> > >> >> >> > > >> >> >> >> > >> >> >> >> +}; > >> >> >> >> + > >> >> >> >> +struct A { > >> >> >> >> + A &operator=(const A &) = default; > >> >> >> > > >> >> >> > > >> >> >> > Is the fix/codepath specifically about explicitly defaulted ops? > >> >> >> > >> >> >> yes -- explicitly defaulted. There are some test coverage already > >> >> >> for > >> >> >> implicitly declared ctors (but not assignment op -- probably worth > >> >> >> adding some testing too). > >> >> > > >> >> > > >> >> > Hmm - are you sure there's no common codepath that would cover the > >> >> > explicitly defaulted or implicitly defaulted ops together in one > go? > >> >> > >> >> Sorry I am not sure what you mean here. > >> > Is there some part of Clang that is responsible for generating both > >> > implicitly defaulted and explicitly defaulted move/copy ops that could > >> > handle this case, rather than apparently handling the implicit and > >> > explicit > >> > cases separately (it seems they're being handled separately if the > >> > implicit > >> > case worked before and you added code (rather than moving code) to fix > >> > the > >> > explicit case - it sounds like we now have two bits of code, one for > >> > implicit and one for explicit - perhaps there's a single bit of code > >> > that we > >> > could write that would handle both?) > >> > >> The codegen paths are different -- otherwise as you commented, the > >> implicit case would have been broken too. > >> > >> Refactoring FE code to handle both is probably beyond the scope of > >> this fix. Having a good test case here will exactly help avoid > >> regression if that happens in the future. > >> > >> David > >> > >> > > >> > - David > >> > > >> >> > >> >> > >> >> David > >> >> > >> >> > > >> >> >> > >> >> >> > >> >> >> > Or just any > >> >> >> > compiler-generated ones? (you could drop these lines if it's > about > >> >> >> > any > >> >> >> > compiler-generated ones, might be simpler/more obvious that it's > >> >> >> > not > >> >> >> > about > >> >> >> > the "= default" feature) > >> >> >> > >> >> >> Other compiler generated ones are handled differently. > >> >> >> > >> >> >> thanks, > >> >> >> > >> >> >> David > >> >> >> > >> >> >> > > >> >> >> >> > >> >> >> >> + // PGOGEN: define {{.*}}@_ZN1AaSERKS_( > >> >> >> >> + // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_ > >> >> >> >> + // PGOGEN: {{.*}}add{{.*}}%pgocount, 1 > >> >> >> >> + // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_ > >> >> >> >> + A &operator=(A &&) = default; > >> >> >> >> > >> >> >> >> + // PGOGEN: define {{.*}}@_ZN1AaSEOS_ > >> >> >> >> + // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSEOS_ > >> >> >> >> + // PGOGEN: {{.*}}add{{.*}}%pgocount, 1 > >> >> >> >> + // PGOGEN: store{{.*}}@__profc__ZN1AaSEOS_ > >> >> >> >> + > >> >> >> >> + // Check that coverage mapping includes 6 function records > >> >> >> >> including > >> >> >> >> the > >> >> >> >> + // defaulted copy and move operators: A::operator= > >> >> >> >> + // COVMAP: @__llvm_coverage_mapping = {{.*}} { { i32, i32, > >> >> >> >> i32, > >> >> >> >> i32 > >> >> >> >> }, > >> >> >> >> [5 x <{{.*}}>], > >> >> >> >> + B b; > >> >> >> >> +}; > >> >> >> >> + > >> >> >> >> +int main() { > >> >> >> >> + A a1, a2; > >> >> >> >> + a1 = a2; > >> >> >> >> + a2 = static_cast<A &&>(a1); > >> >> >> > > >> >> >> > > >> >> >> > An option, though not necessarily better, would be to just take > >> >> >> > the > >> >> >> > address > >> >> >> > of the special members: > >> >> >> > > >> >> >> > auto (B::*x)(const B&) = &bar::operator=; > >> >> >> > auto (B::*x)(B&&) = &bar::operator=; > >> >> >> > > >> >> >> > In short, what I'm picturing, in total: > >> >> >> > > >> >> >> > struct A { > >> >> >> > A &operator=(const A&); > >> >> >> > A &operator=(A&&); > >> >> >> > }; > >> >> >> > > >> >> >> > struct B { > >> >> >> > A a; > >> >> >> > }; > >> >> >> > > >> >> >> > auto (B::*x)(const B&) = &B::operator=; > >> >> >> > auto (B::*x)(B&&) = &B::operator=; > >> >> >> > > >> >> >> > Also, this test should probably be in clang, since it's a clang > >> >> >> > code > >> >> >> > change/fix. > >> >> >> > > >> >> >> > > >> >> >> >> > >> >> >> >> + return 0; > >> >> >> >> +} > >> >> >> >> Index: lib/CodeGen/CGClass.cpp > >> >> >> >> > >> >> >> >> > =================================================================== > >> >> >> >> --- lib/CodeGen/CGClass.cpp > >> >> >> >> +++ lib/CodeGen/CGClass.cpp > >> >> >> >> @@ -1608,6 +1608,7 @@ > >> >> >> >> > >> >> >> >> LexicalScope Scope(*this, RootCS->getSourceRange()); > >> >> >> >> > >> >> >> >> + incrementProfileCounter(RootCS); > >> >> >> >> AssignmentMemcpyizer AM(*this, AssignOp, Args); > >> >> >> >> for (auto *I : RootCS->body()) > >> >> >> >> AM.emitAssignment(I); > >> >> >> >> > >> >> >> >> > >> >> >> >> > >> >> >> >> _______________________________________________ > >> >> >> >> llvm-commits mailing list > >> >> >> >> llvm-comm...@lists.llvm.org > >> >> >> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits > >> >> >> >> > >> >> >> > > >> >> > > >> >> > > >> > > >> > > > > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits