<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li <span dir="ltr"><<a href="mailto:davidxl@google.com" target="_blank">davidxl@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
> wrote:<br>
>><br>
>> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>> ><br>
>> ><br>
>> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits<br>
>> > <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
>> >><br>
>> >> davidxl updated this revision to Diff 47217.<br>
>> >> davidxl added a comment.<br>
>> >><br>
>> >> Simplified test case suggested by Vedant.<br>
>> >><br>
>> >><br>
>> >> <a href="http://reviews.llvm.org/D16947" rel="noreferrer" target="_blank">http://reviews.llvm.org/D16947</a><br>
>> >><br>
>> >> Files:<br>
>> >> lib/CodeGen/CGClass.cpp<br>
>> >> test/Profile/def-assignop.cpp<br>
>> >><br>
>> >> Index: test/Profile/def-assignop.cpp<br>
>> >> ===================================================================<br>
>> >> --- test/Profile/def-assignop.cpp<br>
>> >> +++ test/Profile/def-assignop.cpp<br>
>> >> @@ -0,0 +1,32 @@<br>
>> >> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple<br>
>> >> x86_64-unknown-linux-gnu<br>
>> >> -main-file-name def-assignop.cpp -o - -emit-llvm<br>
>> >> -fprofile-instrument=clang<br>
>> >> | FileCheck --check-prefix=PGOGEN %s<br>
>> >> +// RUN: %clang_cc1 -x c++ -std=c++11 %s -triple<br>
>> >> x86_64-unknown-linux-gnu<br>
>> >> -main-file-name def-assignop.cpp -o - -emit-llvm<br>
>> >> -fprofile-instrument=clang<br>
>> >> -fcoverage-mapping | FileCheck --check-prefix=COVMAP %s<br>
>> >> +<br>
>> >> +struct B {<br>
>> >> + void operator=(const B &b) {}<br>
>> >> + void operator=(const B &&b) {}<br>
>> ><br>
>> ><br>
>> > Probably best to make these canonical to avoid confusion:<br>
>> ><br>
>> > B &operator=(const B&);<br>
>> > B &operator=(B&&);<br>
>> ><br>
>> > (& they don't need definitions - just declarations)<br>
>><br>
>> Will change.<br>
>><br>
>> ><br>
>> > Also, neither of these are the move /constructor/, just the move<br>
>> > operator.<br>
>> > Not sure if Vedant just used the wrong terminology, or whether it's<br>
>> > worth<br>
>> > testing the move/copy ctors too, to check that they do the right thing<br>
>> > as<br>
>><br>
>> I added tests for copy ctors, and plan to add move ctor test soon.<br>
>><br>
>> > well. (if all of these things use the same codepath, I don't see a great<br>
>> > benefit in having separate tests for them (but you can add them here if<br>
>> > you<br>
>> > like) - I'm just suggesting a manual verification in case those need a<br>
>> > separate fix<br>
>><br>
>> the ctor and assignment op do not share the same path -- the ctor path<br>
>> is working as expected without the fix -- or do you mean there is no<br>
>> need to cover both copy and move variants?<br>
><br>
><br>
> I wouldn't necessarily bother testing multiple instances of the same<br>
> codepath (so the copy and move ctor for example) - but 2 instances is no big<br>
> deal (if there were several more, I might be inclined to just test one as a<br>
> representative sample). I don't mind either way, though. The number is small<br>
> & the test cases are arguably distinct.<br>
<br>
</div></div>Sorry I disagree with your general statement here. I treat such test<br>
cases as 'black box testing' that do not know about the internal<br>
implementation (code path). It may or may not share the same code path<br>
today -- same is true in the future.<br></blockquote><div><br></div><div>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 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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class=""><br>
><br>
>><br>
>> ><br>
>> >><br>
>> >> +};<br>
>> >> +<br>
>> >> +struct A {<br>
>> >> + A &operator=(const A &) = default;<br>
>> ><br>
>> ><br>
>> > Is the fix/codepath specifically about explicitly defaulted ops?<br>
>><br>
>> yes -- explicitly defaulted. There are some test coverage already for<br>
>> implicitly declared ctors (but not assignment op -- probably worth<br>
>> adding some testing too).<br>
><br>
><br>
> Hmm - are you sure there's no common codepath that would cover the<br>
> explicitly defaulted or implicitly defaulted ops together in one go?<br>
<br>
</span>Sorry I am not sure what you mean here.<br></blockquote><div><br></div><div>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?)<br><br>- David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<span class="HOEnZb"><font color="#888888"><br>
David<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
><br>
>><br>
>><br>
>> > Or just any<br>
>> > compiler-generated ones? (you could drop these lines if it's about any<br>
>> > compiler-generated ones, might be simpler/more obvious that it's not<br>
>> > about<br>
>> > the "= default" feature)<br>
>><br>
>> Other compiler generated ones are handled differently.<br>
>><br>
>> thanks,<br>
>><br>
>> David<br>
>><br>
>> ><br>
>> >><br>
>> >> + // PGOGEN: define {{.*}}@_ZN1AaSERKS_(<br>
>> >> + // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSERKS_<br>
>> >> + // PGOGEN: {{.*}}add{{.*}}%pgocount, 1<br>
>> >> + // PGOGEN: store{{.*}}@__profc__ZN1AaSERKS_<br>
>> >> + A &operator=(A &&) = default;<br>
>> >><br>
>> >> + // PGOGEN: define {{.*}}@_ZN1AaSEOS_<br>
>> >> + // PGOGEN: %pgocount = load {{.*}} @__profc__ZN1AaSEOS_<br>
>> >> + // PGOGEN: {{.*}}add{{.*}}%pgocount, 1<br>
>> >> + // PGOGEN: store{{.*}}@__profc__ZN1AaSEOS_<br>
>> >> +<br>
>> >> + // Check that coverage mapping includes 6 function records including<br>
>> >> the<br>
>> >> + // defaulted copy and move operators: A::operator=<br>
>> >> + // COVMAP: @__llvm_coverage_mapping = {{.*}} { { i32, i32, i32, i32<br>
>> >> },<br>
>> >> [5 x <{{.*}}>],<br>
>> >> + B b;<br>
>> >> +};<br>
>> >> +<br>
>> >> +int main() {<br>
>> >> + A a1, a2;<br>
>> >> + a1 = a2;<br>
>> >> + a2 = static_cast<A &&>(a1);<br>
>> ><br>
>> ><br>
>> > An option, though not necessarily better, would be to just take the<br>
>> > address<br>
>> > of the special members:<br>
>> ><br>
>> > auto (B::*x)(const B&) = &bar::operator=;<br>
>> > auto (B::*x)(B&&) = &bar::operator=;<br>
>> ><br>
>> > In short, what I'm picturing, in total:<br>
>> ><br>
>> > struct A {<br>
>> > A &operator=(const A&);<br>
>> > A &operator=(A&&);<br>
>> > };<br>
>> ><br>
>> > struct B {<br>
>> > A a;<br>
>> > };<br>
>> ><br>
>> > auto (B::*x)(const B&) = &B::operator=;<br>
>> > auto (B::*x)(B&&) = &B::operator=;<br>
>> ><br>
>> > Also, this test should probably be in clang, since it's a clang code<br>
>> > change/fix.<br>
>> ><br>
>> ><br>
>> >><br>
>> >> + return 0;<br>
>> >> +}<br>
>> >> Index: lib/CodeGen/CGClass.cpp<br>
>> >> ===================================================================<br>
>> >> --- lib/CodeGen/CGClass.cpp<br>
>> >> +++ lib/CodeGen/CGClass.cpp<br>
>> >> @@ -1608,6 +1608,7 @@<br>
>> >><br>
>> >> LexicalScope Scope(*this, RootCS->getSourceRange());<br>
>> >><br>
>> >> + incrementProfileCounter(RootCS);<br>
>> >> AssignmentMemcpyizer AM(*this, AssignOp, Args);<br>
>> >> for (auto *I : RootCS->body())<br>
>> >> AM.emitAssignment(I);<br>
>> >><br>
>> >><br>
>> >><br>
>> >> _______________________________________________<br>
>> >> llvm-commits mailing list<br>
>> >> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
>> >> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
>> >><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>