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