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. > > > >> > >> +}; > >> + > >> +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? > > > 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
