<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 9, 2016 at 11:41 AM, 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"><span class="">On Tue, Feb 9, 2016 at 11:30 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Tue, Feb 9, 2016 at 11:26 AM, Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
> wrote:<br>
>><br>
>> On Tue, Feb 9, 2016 at 11:14 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>> ><br>
>> ><br>
>> > On Mon, Feb 8, 2016 at 9:23 PM, Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
>> > wrote:<br>
>> >><br>
>> >> Wrong in the sense the the coverage result for the default operators<br>
>> >> (the line where they are declared) is marked as if they are not called<br>
>> >> which can be confusing to the user.<br>
>> ><br>
>> ><br>
>> > Presumably a user would have the same problem with implicit ops - the<br>
>> > class<br>
>> > header/name would be marked as if there was code that was not called<br>
>> > there?<br>
>><br>
>> that would be confusing though -- as data of many implicitly declared<br>
>> functions will be lumped together and user won't know what that is .<br>
><br>
><br>
> Presumably it's still going to be confusing, though - the line table will<br>
> record that line and the counter won't be there, so the header of the type<br>
> will be marked in red & a user worried about coverage may go through some<br>
> contortions to try to discover and cover that code, no? (even though it may<br>
> already be covered & is just being reported incorrectly due to their being<br>
> no counters)<br>
<br>
</span>coverage mapping does not use the debug line table for this purpose --<br>
it encodes line info in source regions of its own format. Marking<br>
class header as read will be super confusing if it happens (but does<br>
not).<br></blockquote><div><br></div><div>Then why would it be a problem to omit the defaulted versions? It won't be marked "as if they are not called" (it won't show up red) it'll just be marked as if there's no code there, right?<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Further discussion is welcome, but I am going to commit this change soon.<br></blockquote><div><br></div><div>As for the review - I'd still request that the test be in Clang, where the code change is. Please defer adding tests like this to compiler-rt until we've had some discussion with compiler-rt folks (I tried to rope Alexey into one of these threads, not sure if it got lost in the noise), perhaps on llvm-dev. Maybe I'm confused about what testing should go in compiler-rt, but we can defer that to a discussion as there should be a test in Clang regardless of whether there's also testing for this specific feature in compiler-rt.<br><br>- David</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
thanks,<br>
<br>
David<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
>><br>
>><br>
>> David<br>
>><br>
>> ><br>
>> > - David<br>
>> ><br>
>> >><br>
>> >><br>
>> >> David<br>
>> >><br>
>> >> On Mon, Feb 8, 2016 at 9:09 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> wrote:<br>
>> >> ><br>
>> >> ><br>
>> >> > On Mon, Feb 8, 2016 at 9:00 PM, Xinliang David Li<br>
>> >> > <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
>> >> > wrote:<br>
>> >> >><br>
>> >> >> On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> >> wrote:<br>
>> >> >> ><br>
>> >> >> > On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li<br>
>> >> >> > <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
>> >> >> > wrote:<br>
>> >> >> >><br>
>> >> >> >> I took a look at the problem. The implicitly defaulted operators<br>
>> >> >> >> should not be instrumented as specified -- I actually I just<br>
>> >> >> >> added<br>
>> >> >> >> the<br>
>> >> >> >> new test case for that (checking profile counter not generated)<br>
>> >> >> >> right<br>
>> >> >> >> after my previous reply and it still passes with this patch. The<br>
>> >> >> >> reason is that those operators have 'implicit' bit set, and<br>
>> >> >> >> profile<br>
>> >> >> >> instrumentation in FE is implemented in two stages: 1) counter<br>
>> >> >> >> assignment; 2) actual transformation.  For methods with implicit<br>
>> >> >> >> bit<br>
>> >> >> >> set, step 1) is skipped as designed, so step 2) simply becomes a<br>
>> >> >> >> no-op.<br>
>> >> >> >><br>
>> >> >> >> In short, the test case still needs explicit '=default', and the<br>
>> >> >> >> implicit case is covered elsewhere.<br>
>> >> >> ><br>
>> >> >> ><br>
>> >> >> > OK, thanks for the explanation!<br>
>> >> >> ><br>
>> >> >> > Why is that the case, though - why would an implicitly default<br>
>> >> >> > function<br>
>> >> >> > be<br>
>> >> >> > any different from a profile (& profile-guided-optimization)<br>
>> >> >> > perspective<br>
>> >> >> > from an explicitly defaulted one?<br>
>> >> >><br>
>> >> >> There are two factors to consider -- PGO and coverage testing.<br>
>> >> >> Implicitly declared functions are usually small/single BB so<br>
>> >> >> instrumenting them does not bring too much benefit (they will be<br>
>> >> >> inlined most of the cases any way) while increasing instrumentation<br>
>> >> >> overhead. They are not needed for coveraging test either (as there<br>
>> >> >> are<br>
>> >> >> no source lines to cover). This is a good tuning heuristic in most<br>
>> >> >> cases, but can get wrong sometimes (IR based late instrumentation is<br>
>> >> >> more focused on performance thus not depending on this tuning).<br>
>> >> >><br>
>> >> >> Explicitly defaulted ones are different in the sense that if they<br>
>> >> >> are<br>
>> >> >> not instrumented, the coverage result will be wrong.<br>
>> >> ><br>
>> >> ><br>
>> >> > wrong in what way? Both functions (explicitly or implicitly<br>
>> >> > defaulted)<br>
>> >> > will<br>
>> >> > be emitted, with line tables (looks like the = defaulted one is<br>
>> >> > attributed<br>
>> >> > to the line where the = default was written, and the implicitly<br>
>> >> > defaulted<br>
>> >> > one is attributed to wherever the class name is written)<br>
>> >> ><br>
>> >> > - David<br>
>> >> ><br>
>> >> >><br>
>> >> >><br>
>> >> >> thanks,<br>
>> >> >><br>
>> >> >> David<br>
>> >> >><br>
>> >> >> ><br>
>> >> >> >><br>
>> >> >> >><br>
>> >> >> >> thanks,<br>
>> >> >> >><br>
>> >> >> >> David<br>
>> >> >> >><br>
>> >> >> >> On Mon, Feb 8, 2016 at 5:23 PM, David Blaikie<br>
>> >> >> >> <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> >> >> wrote:<br>
>> >> >> >> ><br>
>> >> >> >> ><br>
>> >> >> >> > On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li<br>
>> >> >> >> > <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
>> >> >> >> > wrote:<br>
>> >> >> >> >><br>
>> >> >> >> >> ha! somehow I kept thinking you are referring to implicit<br>
>> >> >> >> >> declared<br>
>> >> >> >> >> ctors.<br>
>> >> >> >> ><br>
>> >> >> >> ><br>
>> >> >> >> > Ah, glad we figured out the disconnect - thanks for bearing<br>
>> >> >> >> > with<br>
>> >> >> >> > me!<br>
>> >> >> >> ><br>
>> >> >> >> >><br>
>> >> >> >> >><br>
>> >> >> >> >> From your test case, it is seems that the implicit copy/move<br>
>> >> >> >> >> op<br>
>> >> >> >> >> is<br>
>> >> >> >> >> also broken and is fixed by this patch too. That means  a<br>
>> >> >> >> >> missing<br>
>> >> >> >> >> test<br>
>> >> >> >> >> case to me.  Will update the case when verified.<br>
>> >> >> >> ><br>
>> >> >> >> ><br>
>> >> >> >> > Again, this is a case where I'd probably just simplify the<br>
>> >> >> >> > test,<br>
>> >> >> >> > as I<br>
>> >> >> >> > asked<br>
>> >> >> >> > earlier in the thread (I asked if it mattered if the op was<br>
>> >> >> >> > explicitly<br>
>> >> >> >> > or<br>
>> >> >> >> > implicitly defaulted (& your response: "> Is the fix/codepath<br>
>> >> >> >> > specifically<br>
>> >> >> >> > about explicitly defaulted ops?<br>
>> >> >> >> ><br>
>> >> >> >> > yes -- explicitly defaulted. There are some test coverage<br>
>> >> >> >> > already<br>
>> >> >> >> > for<br>
>> >> >> >> > implicitly declared ctors (but not assignment op -- probably<br>
>> >> >> >> > worth<br>
>> >> >> >> > adding some testing too).")<br>
>> >> >> >> ><br>
>> >> >> >> > So I'd just simplify the test by removing the "= default" - I<br>
>> >> >> >> > don't<br>
>> >> >> >> > think<br>
>> >> >> >> > there's value in testing both the explicit default and implicit<br>
>> >> >> >> > default<br>
>> >> >> >> > if<br>
>> >> >> >> > it's just the default-y-ness that's relevant here. Otherwise we<br>
>> >> >> >> > could<br>
>> >> >> >> > end up<br>
>> >> >> >> > testing all sorts of ways of writing/interacting with dtors<br>
>> >> >> >> > which<br>
>> >> >> >> > wouldn't<br>
>> >> >> >> > be relevant to the code/fix/etc.<br>
>> >> >> >> ><br>
>> >> >> >> > This seems like the obvious test for the behavior:<br>
>> >> >> >> ><br>
>> >> >> >> > struct foo {<br>
>> >> >> >> >   // non-trivial ops<br>
>> >> >> >> >   foo &operator=(const foo&);<br>
>> >> >> >> >   foo &operator=(foo&&);<br>
>> >> >> >> > };<br>
>> >> >> >> ><br>
>> >> >> >> > struct bar {<br>
>> >> >> >> >   foo f; // or derive bar from foo, but I think the member<br>
>> >> >> >> > version<br>
>> >> >> >> > is<br>
>> >> >> >> > simpler<br>
>> >> >> >> > };<br>
>> >> >> >> ><br>
>> >> >> >> > // force emission of bar's implicit special members, one way or<br>
>> >> >> >> > another:<br>
>> >> >> >> > bar &(bar::*x)(const bar&) = &bar::operator=;<br>
>> >> >> >> > bar &(bar::*x)(bar&&) = &bar::operator=;<br>
>> >> >> >> ><br>
>> >> >> >> > (or just call them as you had in your test case - but that<br>
>> >> >> >> > produces<br>
>> >> >> >> > more<br>
>> >> >> >> > code, etc in the module, extra functions/profile counters/etc)<br>
>> >> >> >> ><br>
>> >> >> >> > - Dave<br>
>> >> >> >> ><br>
>> >> >> >> >><br>
>> >> >> >> >><br>
>> >> >> >> >> thanks,<br>
>> >> >> >> >><br>
>> >> >> >> >> David<br>
>> >> >> >> >><br>
>> >> >> >> >><br>
>> >> >> >> >> On Mon, Feb 8, 2016 at 4:58 PM, David Blaikie<br>
>> >> >> >> >> <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> >> >> >> wrote:<br>
>> >> >> >> >> ><br>
>> >> >> >> >> ><br>
>> >> >> >> >> > On Mon, Feb 8, 2016 at 4:31 PM, Xinliang David Li<br>
>> >> >> >> >> > <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
>> >> >> >> >> > wrote:<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> On Mon, Feb 8, 2016 at 4:05 PM, David Blaikie<br>
>> >> >> >> >> >> <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> >> >> >> >> wrote:<br>
>> >> >> >> >> >> ><br>
>> >> >> >> >> >> ><br>
>> >> >> >> >> >> > On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li<br>
>> >> >> >> >> >> > <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
>> >> >> >> >> >> > wrote:<br>
>> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> To be clear, you are suggesting breaking the test into<br>
>> >> >> >> >> >> >> two<br>
>> >> >> >> >> >> >> (one<br>
>> >> >> >> >> >> >> for<br>
>> >> >> >> >> >> >> copy, and one for move) ? I am totally fine with that.<br>
>> >> >> >> >> >> ><br>
>> >> >> >> >> >> ><br>
>> >> >> >> >> >> > Nah, no need to split the test case - we try to keep the<br>
>> >> >> >> >> >> > number<br>
>> >> >> >> >> >> > of<br>
>> >> >> >> >> >> > test<br>
>> >> >> >> >> >> > files down (& group related tests into a single file) to<br>
>> >> >> >> >> >> > reduce<br>
>> >> >> >> >> >> > test<br>
>> >> >> >> >> >> > execution time (a non-trivial about of check time is<br>
>> >> >> >> >> >> > spent<br>
>> >> >> >> >> >> > in<br>
>> >> >> >> >> >> > process<br>
>> >> >> >> >> >> > overhead).<br>
>> >> >> >> >> >> ><br>
>> >> >> >> >> >> >><br>
>> >> >> >> >> >> >>   I thought you<br>
>> >> >> >> >> >> >> suggested removing the testing of move/op case because<br>
>> >> >> >> >> >> >> they<br>
>> >> >> >> >> >> >> might<br>
>> >> >> >> >> >> >> share the same code path (clang's implementation) as the<br>
>> >> >> >> >> >> >> copy/op.<br>
>> >> >> >> >> >> ><br>
>> >> >> >> >> >> ><br>
>> >> >> >> >> >> > I was suggesting that two cases is no big deal whether<br>
>> >> >> >> >> >> > you<br>
>> >> >> >> >> >> > test<br>
>> >> >> >> >> >> > both<br>
>> >> >> >> >> >> > or<br>
>> >> >> >> >> >> > test<br>
>> >> >> >> >> >> > one if they're the same codepath - if there were 5/many<br>
>> >> >> >> >> >> > more<br>
>> >> >> >> >> >> > things<br>
>> >> >> >> >> >> > that<br>
>> >> >> >> >> >> > shared the same codepath, I'd generally suggest testing a<br>
>> >> >> >> >> >> > representative<br>
>> >> >> >> >> >> > sample (possibly just a single one) rather than testing<br>
>> >> >> >> >> >> > every<br>
>> >> >> >> >> >> > client<br>
>> >> >> >> >> >> > of<br>
>> >> >> >> >> >> > the<br>
>> >> >> >> >> >> > same code.<br>
>> >> >> >> >> >> ><br>
>> >> >> >> >> >> > Feel free to leave the two here as-is. (though if we're<br>
>> >> >> >> >> >> > talking<br>
>> >> >> >> >> >> > about<br>
>> >> >> >> >> >> > test<br>
>> >> >> >> >> >> > granularity, it's probably worth just putting these cases<br>
>> >> >> >> >> >> > in<br>
>> >> >> >> >> >> > the<br>
>> >> >> >> >> >> > same<br>
>> >> >> >> >> >> > file/type/etc as the ctor cases you mentioned were<br>
>> >> >> >> >> >> > already<br>
>> >> >> >> >> >> > covered)<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> There is a balance somewhere:<br>
>> >> >> >> >> >> 1) for small test cases like this, the overhead mainly<br>
>> >> >> >> >> >> comes<br>
>> >> >> >> >> >> from<br>
>> >> >> >> >> >> test<br>
>> >> >> >> >> >> set up cost -- adding additional incremental test in the<br>
>> >> >> >> >> >> same<br>
>> >> >> >> >> >> file<br>
>> >> >> >> >> >> probably almost comes for free (in terms of cost). However<br>
>> >> >> >> >> >> 2) having too many cases grouped together also reduces the<br>
>> >> >> >> >> >> debuggability when some test fails.<br>
>> >> >> >> >> ><br>
>> >> >> >> >> ><br>
>> >> >> >> >> > Yep, for sure. In this case, testing the ctors and<br>
>> >> >> >> >> > assignment<br>
>> >> >> >> >> > ops<br>
>> >> >> >> >> > in<br>
>> >> >> >> >> > one<br>
>> >> >> >> >> > file's probably not a bad tradeoff (you can see how Clang<br>
>> >> >> >> >> > groups<br>
>> >> >> >> >> > its<br>
>> >> >> >> >> > tests -<br>
>> >> >> >> >> > a file per language feature in many cases, exploring the<br>
>> >> >> >> >> > myriad<br>
>> >> >> >> >> > ways<br>
>> >> >> >> >> > the<br>
>> >> >> >> >> > feature can be used - this doesn't always work spectacularly<br>
>> >> >> >> >> > (when<br>
>> >> >> >> >> > you<br>
>> >> >> >> >> > can't<br>
>> >> >> >> >> > order the IR emission to happen mostly in the order that the<br>
>> >> >> >> >> > source<br>
>> >> >> >> >> > is<br>
>> >> >> >> >> > written (rather being interleaved))<br>
>> >> >> >> >> ><br>
>> >> >> >> >> > Anyway, up to you - that part isn't something I'm terribly<br>
>> >> >> >> >> > worried<br>
>> >> >> >> >> > about<br>
>> >> >> >> >> > either way.<br>
>> >> >> >> >> ><br>
>> >> >> >> >> >><br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> ><br>
>> >> >> >> >> >> > & I'm still curious/wondering if there's a common<br>
>> >> >> >> >> >> > codepath<br>
>> >> >> >> >> >> > that<br>
>> >> >> >> >> >> > would<br>
>> >> >> >> >> >> > provide a simpler fix/code that solved both implicit and<br>
>> >> >> >> >> >> > explicitly<br>
>> >> >> >> >> >> > defaulted ops.<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> I may take a look at that when I find time -- but there is<br>
>> >> >> >> >> >> no<br>
>> >> >> >> >> >> guarantee<br>
>> >> >> >> >> >> :)<br>
>> >> >> >> >> ><br>
>> >> >> >> >> ><br>
>> >> >> >> >> > A quick test of putting "assert(false)" in<br>
>> >> >> >> >> > emitImplicitAssignmentOperatorBody and running Clang over<br>
>> >> >> >> >> > this<br>
>> >> >> >> >> > code:<br>
>> >> >> >> >> ><br>
>> >> >> >> >> > struct foo {<br>
>> >> >> >> >> >   foo &operator=(const foo &);<br>
>> >> >> >> >> > };<br>
>> >> >> >> >> ><br>
>> >> >> >> >> > struct bar {<br>
>> >> >> >> >> >   foo f;<br>
>> >> >> >> >> > };<br>
>> >> >> >> >> ><br>
>> >> >> >> >> > auto (bar::*x)(const bar&) = &bar::operator=;<br>
>> >> >> >> >> ><br>
>> >> >> >> >> > Fires the assertion - this seems to me to indicate that the<br>
>> >> >> >> >> > codepath<br>
>> >> >> >> >> > you<br>
>> >> >> >> >> > changed is used for both the explicitly (based on the change<br>
>> >> >> >> >> > fixing<br>
>> >> >> >> >> > your<br>
>> >> >> >> >> > test case) and implicitly defaulted (based on my test case)<br>
>> >> >> >> >> > cases.<br>
>> >> >> >> >> ><br>
>> >> >> >> >> > Is it possible that you end up with duplicate counters by<br>
>> >> >> >> >> > accident<br>
>> >> >> >> >> > in<br>
>> >> >> >> >> > this<br>
>> >> >> >> >> > path, then? Or at least that whatever codepath was handling<br>
>> >> >> >> >> > the<br>
>> >> >> >> >> > implicitly<br>
>> >> >> >> >> > defaulted ones is now redundant with this one?<br>
>> >> >> >> >> ><br>
>> >> >> >> >> > Actually, so far as I can tell this doesn't work for<br>
>> >> >> >> >> > implicitly<br>
>> >> >> >> >> > defaulted<br>
>> >> >> >> >> > move ops (the above test case doesn't have an add pgocount<br>
>> >> >> >> >> > in<br>
>> >> >> >> >> > it)<br>
>> >> >> >> >> > -<br>
>> >> >> >> >> > perhaps<br>
>> >> >> >> >> > I'm missing something/doing it wrong? or was just not<br>
>> >> >> >> >> > communicating<br>
>> >> >> >> >> > clearly<br>
>> >> >> >> >> > regarding explicit versus implicitly defaulted special<br>
>> >> >> >> >> > members.<br>
>> >> >> >> >> ><br>
>> >> >> >> >> > - Dave<br>
>> >> >> >> >> ><br>
>> >> >> >> >> >><br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> thanks,<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> David<br>
>> >> >> >> >> >><br>
>> >> >> >> >> >><br>
>> >> >> >> >> >><br>
>> >> >> >> >> >> ><br>
>> >> >> >> >> >> > - Dave<br>
>> >> >> >> >> >> ><br>
>> >> >> >> >> >> >><br>
>> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> thanks,<br>
>> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> David<br>
>> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> On Mon, Feb 8, 2016 at 3:52 PM, David Blaikie<br>
>> >> >> >> >> >> >> <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> >> >> >> >> >> wrote:<br>
>> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> > On Mon, Feb 8, 2016 at 3:46 PM, Xinliang David Li<br>
>> >> >> >> >> >> >> > <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
>> >> >> >> >> >> >> > wrote:<br>
>> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie<br>
>> >> >> >> >> >> >> >> <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> >> >> >> >> >> >> wrote:<br>
>> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> > On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li<br>
>> >> >> >> >> >> >> >> > <<a href="mailto:davidxl@google.com">davidxl@google.com</a>><br>
>> >> >> >> >> >> >> >> > wrote:<br>
>> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie<br>
>> >> >> >> >> >> >> >> >> <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> >> >> >> >> >> >> >> wrote:<br>
>> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David<br>
>> >> >> >> >> >> >> >> >> > Li<br>
>> >> >> >> >> >> >> >> >> > <<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<br>
>> >> >> >> >> >> >> >> >> >> <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>><br>
>> >> >> >> >> >> >> >> >> >> wrote:<br>
>> >> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> >> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via<br>
>> >> >> >> >> >> >> >> >> >> > 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>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >><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<br>
>> >> >> >> >> >> >> >> >> >> >> -triple<br>
>> >> >> >> >> >> >> >> >> >> >> x86_64-unknown-linux-gnu<br>
>> >> >> >> >> >> >> >> >> >> >> -main-file-name def-assignop.cpp -o -<br>
>> >> >> >> >> >> >> >> >> >> >> -emit-llvm<br>
>> >> >> >> >> >> >> >> >> >> >> -fprofile-instrument=clang<br>
>> >> >> >> >> >> >> >> >> >> >> | FileCheck --check-prefix=PGOGEN %s<br>
>> >> >> >> >> >> >> >> >> >> >> +// RUN: %clang_cc1 -x c++ -std=c++11 %s<br>
>> >> >> >> >> >> >> >> >> >> >> -triple<br>
>> >> >> >> >> >> >> >> >> >> >> x86_64-unknown-linux-gnu<br>
>> >> >> >> >> >> >> >> >> >> >> -main-file-name def-assignop.cpp -o -<br>
>> >> >> >> >> >> >> >> >> >> >> -emit-llvm<br>
>> >> >> >> >> >> >> >> >> >> >> -fprofile-instrument=clang<br>
>> >> >> >> >> >> >> >> >> >> >> -fcoverage-mapping | FileCheck<br>
>> >> >> >> >> >> >> >> >> >> >> --check-prefix=COVMAP<br>
>> >> >> >> >> >> >> >> >> >> >> %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<br>
>> >> >> >> >> >> >> >> >> >> > avoid<br>
>> >> >> >> >> >> >> >> >> >> > confusion:<br>
>> >> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> >> > B &operator=(const B&);<br>
>> >> >> >> >> >> >> >> >> >> > B &operator=(B&&);<br>
>> >> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> >> > (& they don't need definitions - just<br>
>> >> >> >> >> >> >> >> >> >> > declarations)<br>
>> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> Will change.<br>
>> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> >> > Also, neither of these are the move<br>
>> >> >> >> >> >> >> >> >> >> > /constructor/,<br>
>> >> >> >> >> >> >> >> >> >> > just<br>
>> >> >> >> >> >> >> >> >> >> > the<br>
>> >> >> >> >> >> >> >> >> >> > move<br>
>> >> >> >> >> >> >> >> >> >> > operator.<br>
>> >> >> >> >> >> >> >> >> >> > Not sure if Vedant just used the wrong<br>
>> >> >> >> >> >> >> >> >> >> > terminology,<br>
>> >> >> >> >> >> >> >> >> >> > or<br>
>> >> >> >> >> >> >> >> >> >> > whether<br>
>> >> >> >> >> >> >> >> >> >> > it's<br>
>> >> >> >> >> >> >> >> >> >> > worth<br>
>> >> >> >> >> >> >> >> >> >> > testing the move/copy ctors too, to check<br>
>> >> >> >> >> >> >> >> >> >> > that<br>
>> >> >> >> >> >> >> >> >> >> > they<br>
>> >> >> >> >> >> >> >> >> >> > do<br>
>> >> >> >> >> >> >> >> >> >> > the<br>
>> >> >> >> >> >> >> >> >> >> > right<br>
>> >> >> >> >> >> >> >> >> >> > thing<br>
>> >> >> >> >> >> >> >> >> >> > as<br>
>> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> I added tests for copy ctors, and plan to add<br>
>> >> >> >> >> >> >> >> >> >> move<br>
>> >> >> >> >> >> >> >> >> >> ctor<br>
>> >> >> >> >> >> >> >> >> >> test<br>
>> >> >> >> >> >> >> >> >> >> soon.<br>
>> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> > well. (if all of these things use the same<br>
>> >> >> >> >> >> >> >> >> >> > codepath, I<br>
>> >> >> >> >> >> >> >> >> >> > don't<br>
>> >> >> >> >> >> >> >> >> >> > see a<br>
>> >> >> >> >> >> >> >> >> >> > great<br>
>> >> >> >> >> >> >> >> >> >> > benefit in having separate tests for them<br>
>> >> >> >> >> >> >> >> >> >> > (but<br>
>> >> >> >> >> >> >> >> >> >> > you<br>
>> >> >> >> >> >> >> >> >> >> > can<br>
>> >> >> >> >> >> >> >> >> >> > add<br>
>> >> >> >> >> >> >> >> >> >> > them<br>
>> >> >> >> >> >> >> >> >> >> > here<br>
>> >> >> >> >> >> >> >> >> >> > if<br>
>> >> >> >> >> >> >> >> >> >> > you<br>
>> >> >> >> >> >> >> >> >> >> > like) - I'm just suggesting a manual<br>
>> >> >> >> >> >> >> >> >> >> > verification<br>
>> >> >> >> >> >> >> >> >> >> > in<br>
>> >> >> >> >> >> >> >> >> >> > case<br>
>> >> >> >> >> >> >> >> >> >> > those<br>
>> >> >> >> >> >> >> >> >> >> > need<br>
>> >> >> >> >> >> >> >> >> >> > a<br>
>> >> >> >> >> >> >> >> >> >> > separate fix<br>
>> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> the ctor and assignment op do not share the<br>
>> >> >> >> >> >> >> >> >> >> same<br>
>> >> >> >> >> >> >> >> >> >> path<br>
>> >> >> >> >> >> >> >> >> >> --<br>
>> >> >> >> >> >> >> >> >> >> the<br>
>> >> >> >> >> >> >> >> >> >> ctor<br>
>> >> >> >> >> >> >> >> >> >> path<br>
>> >> >> >> >> >> >> >> >> >> is working as expected without the fix -- or do<br>
>> >> >> >> >> >> >> >> >> >> you<br>
>> >> >> >> >> >> >> >> >> >> mean<br>
>> >> >> >> >> >> >> >> >> >> there<br>
>> >> >> >> >> >> >> >> >> >> is<br>
>> >> >> >> >> >> >> >> >> >> no<br>
>> >> >> >> >> >> >> >> >> >> need to cover both copy and move variants?<br>
>> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> > I wouldn't necessarily bother testing multiple<br>
>> >> >> >> >> >> >> >> >> > instances<br>
>> >> >> >> >> >> >> >> >> > of<br>
>> >> >> >> >> >> >> >> >> > the<br>
>> >> >> >> >> >> >> >> >> > same<br>
>> >> >> >> >> >> >> >> >> > codepath (so the copy and move ctor for example)<br>
>> >> >> >> >> >> >> >> >> > -<br>
>> >> >> >> >> >> >> >> >> > but<br>
>> >> >> >> >> >> >> >> >> > 2<br>
>> >> >> >> >> >> >> >> >> > instances<br>
>> >> >> >> >> >> >> >> >> > is<br>
>> >> >> >> >> >> >> >> >> > no<br>
>> >> >> >> >> >> >> >> >> > big<br>
>> >> >> >> >> >> >> >> >> > deal (if there were several more, I might be<br>
>> >> >> >> >> >> >> >> >> > inclined<br>
>> >> >> >> >> >> >> >> >> > to<br>
>> >> >> >> >> >> >> >> >> > just<br>
>> >> >> >> >> >> >> >> >> > test<br>
>> >> >> >> >> >> >> >> >> > one<br>
>> >> >> >> >> >> >> >> >> > as a<br>
>> >> >> >> >> >> >> >> >> > representative sample). I don't mind either way,<br>
>> >> >> >> >> >> >> >> >> > though.<br>
>> >> >> >> >> >> >> >> >> > The<br>
>> >> >> >> >> >> >> >> >> > number<br>
>> >> >> >> >> >> >> >> >> > is<br>
>> >> >> >> >> >> >> >> >> > small<br>
>> >> >> >> >> >> >> >> >> > & the test cases are arguably distinct.<br>
>> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> Sorry I disagree with your general statement here.<br>
>> >> >> >> >> >> >> >> >> I<br>
>> >> >> >> >> >> >> >> >> treat<br>
>> >> >> >> >> >> >> >> >> such<br>
>> >> >> >> >> >> >> >> >> test<br>
>> >> >> >> >> >> >> >> >> cases as 'black box testing' that do not know<br>
>> >> >> >> >> >> >> >> >> about<br>
>> >> >> >> >> >> >> >> >> the<br>
>> >> >> >> >> >> >> >> >> internal<br>
>> >> >> >> >> >> >> >> >> implementation (code path). It may or may not<br>
>> >> >> >> >> >> >> >> >> share<br>
>> >> >> >> >> >> >> >> >> the<br>
>> >> >> >> >> >> >> >> >> same<br>
>> >> >> >> >> >> >> >> >> code<br>
>> >> >> >> >> >> >> >> >> path<br>
>> >> >> >> >> >> >> >> >> today -- same is true in the future.<br>
>> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> > While there's merit in both approaches, practically<br>
>> >> >> >> >> >> >> >> > speaking<br>
>> >> >> >> >> >> >> >> > it<br>
>> >> >> >> >> >> >> >> > seems<br>
>> >> >> >> >> >> >> >> > difficult to test in that way in general - any<br>
>> >> >> >> >> >> >> >> > feature<br>
>> >> >> >> >> >> >> >> > could<br>
>> >> >> >> >> >> >> >> > interact<br>
>> >> >> >> >> >> >> >> > with<br>
>> >> >> >> >> >> >> >> > any other.<br>
>> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> The language features are well specified -- so<br>
>> >> >> >> >> >> >> >> writing<br>
>> >> >> >> >> >> >> >> small<br>
>> >> >> >> >> >> >> >> test<br>
>> >> >> >> >> >> >> >> cases to cover them is a general accepted way of<br>
>> >> >> >> >> >> >> >> testing.<br>
>> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> > I'm not sure I follow the distinction you're drawing<br>
>> >> >> >> >> >> >> > between<br>
>> >> >> >> >> >> >> > the<br>
>> >> >> >> >> >> >> > middle<br>
>> >> >> >> >> >> >> > end<br>
>> >> >> >> >> >> >> > optimization tests and the features you're testing<br>
>> >> >> >> >> >> >> > here.<br>
>> >> >> >> >> >> >> > If<br>
>> >> >> >> >> >> >> > the<br>
>> >> >> >> >> >> >> > features<br>
>> >> >> >> >> >> >> > are<br>
>> >> >> >> >> >> >> > relatively independent, even within the same<br>
>> >> >> >> >> >> >> > API/feature<br>
>> >> >> >> >> >> >> > area,<br>
>> >> >> >> >> >> >> > they're<br>
>> >> >> >> >> >> >> > generally tested independently (even two features<br>
>> >> >> >> >> >> >> > within<br>
>> >> >> >> >> >> >> > a<br>
>> >> >> >> >> >> >> > single<br>
>> >> >> >> >> >> >> > middle<br>
>> >> >> >> >> >> >> > end<br>
>> >> >> >> >> >> >> > optimization - a test case is written to ensure that,<br>
>> >> >> >> >> >> >> > say,<br>
>> >> >> >> >> >> >> > ArgumentPromotion<br>
>> >> >> >> >> >> >> > correctly handles debug info, and another that it<br>
>> >> >> >> >> >> >> > correctly<br>
>> >> >> >> >> >> >> > handles<br>
>> >> >> >> >> >> >> > inalloca<br>
>> >> >> >> >> >> >> > (or fp80, etc - just looking at<br>
>> >> >> >> >> >> >> > test/Transforms/ArgumentPromotion)<br>
>> >> >> >> >> >> >> > -<br>
>> >> >> >> >> >> >> > but<br>
>> >> >> >> >> >> >> > we<br>
>> >> >> >> >> >> >> > don't test the matrix of combinations of these<br>
>> >> >> >> >> >> >> > features)<br>
>> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >The LLVM regression suite is far more narrowly<br>
>> >> >> >> >> >> >> >> > targeted<br>
>> >> >> >> >> >> >> >> > than<br>
>> >> >> >> >> >> >> >> > that<br>
>> >> >> >> >> >> >> >> > - we don't test combinations of optimizations, for<br>
>> >> >> >> >> >> >> >> > example -<br>
>> >> >> >> >> >> >> >> > we<br>
>> >> >> >> >> >> >> >> > test<br>
>> >> >> >> >> >> >> >> > each<br>
>> >> >> >> >> >> >> >> > optimization in isolation. The same would be true<br>
>> >> >> >> >> >> >> >> > of<br>
>> >> >> >> >> >> >> >> > two<br>
>> >> >> >> >> >> >> >> > independent<br>
>> >> >> >> >> >> >> >> > features on an interface such as this, I think.<br>
>> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> This is a weakness of the test system -- a problem at<br>
>> >> >> >> >> >> >> >> a<br>
>> >> >> >> >> >> >> >> different<br>
>> >> >> >> >> >> >> >> dimension.<br>
>> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> > If we want to have a discussion about the LLVM<br>
>> >> >> >> >> >> >> > community<br>
>> >> >> >> >> >> >> > testing<br>
>> >> >> >> >> >> >> > methodology, that might be best taken up on llvm-dev<br>
>> >> >> >> >> >> >> > (and<br>
>> >> >> >> >> >> >> > clang-dev).<br>
>> >> >> >> >> >> >> > But<br>
>> >> >> >> >> >> >> > for now, I'd ask that tests in the lit regression<br>
>> >> >> >> >> >> >> > suite<br>
>> >> >> >> >> >> >> > are<br>
>> >> >> >> >> >> >> > generally<br>
>> >> >> >> >> >> >> > as<br>
>> >> >> >> >> >> >> > isolated as possible and test one thing at a time.<br>
>> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >> +};<br>
>> >> >> >> >> >> >> >> >> >> >> +<br>
>> >> >> >> >> >> >> >> >> >> >> +struct A {<br>
>> >> >> >> >> >> >> >> >> >> >> +  A &operator=(const A &) = default;<br>
>> >> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> >> > Is the fix/codepath specifically about<br>
>> >> >> >> >> >> >> >> >> >> > explicitly<br>
>> >> >> >> >> >> >> >> >> >> > defaulted<br>
>> >> >> >> >> >> >> >> >> >> > ops?<br>
>> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> yes -- explicitly defaulted. There are some<br>
>> >> >> >> >> >> >> >> >> >> test<br>
>> >> >> >> >> >> >> >> >> >> coverage<br>
>> >> >> >> >> >> >> >> >> >> already<br>
>> >> >> >> >> >> >> >> >> >> for<br>
>> >> >> >> >> >> >> >> >> >> implicitly declared ctors (but not assignment<br>
>> >> >> >> >> >> >> >> >> >> op<br>
>> >> >> >> >> >> >> >> >> >> --<br>
>> >> >> >> >> >> >> >> >> >> probably<br>
>> >> >> >> >> >> >> >> >> >> worth<br>
>> >> >> >> >> >> >> >> >> >> adding some testing too).<br>
>> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> > Hmm - are you sure there's no common codepath<br>
>> >> >> >> >> >> >> >> >> > that<br>
>> >> >> >> >> >> >> >> >> > would<br>
>> >> >> >> >> >> >> >> >> > cover<br>
>> >> >> >> >> >> >> >> >> > the<br>
>> >> >> >> >> >> >> >> >> > explicitly defaulted or implicitly defaulted ops<br>
>> >> >> >> >> >> >> >> >> > together<br>
>> >> >> >> >> >> >> >> >> > in<br>
>> >> >> >> >> >> >> >> >> > one<br>
>> >> >> >> >> >> >> >> >> > go?<br>
>> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> Sorry I am not sure what you mean here.<br>
>> >> >> >> >> >> >> >> > Is there some part of Clang that is responsible for<br>
>> >> >> >> >> >> >> >> > generating<br>
>> >> >> >> >> >> >> >> > both<br>
>> >> >> >> >> >> >> >> > implicitly defaulted and explicitly defaulted<br>
>> >> >> >> >> >> >> >> > move/copy<br>
>> >> >> >> >> >> >> >> > ops<br>
>> >> >> >> >> >> >> >> > that<br>
>> >> >> >> >> >> >> >> > could<br>
>> >> >> >> >> >> >> >> > handle this case, rather than apparently handling<br>
>> >> >> >> >> >> >> >> > the<br>
>> >> >> >> >> >> >> >> > implicit<br>
>> >> >> >> >> >> >> >> > and<br>
>> >> >> >> >> >> >> >> > explicit<br>
>> >> >> >> >> >> >> >> > cases separately (it seems they're being handled<br>
>> >> >> >> >> >> >> >> > separately<br>
>> >> >> >> >> >> >> >> > if<br>
>> >> >> >> >> >> >> >> > the<br>
>> >> >> >> >> >> >> >> > implicit<br>
>> >> >> >> >> >> >> >> > case worked before and you added code (rather than<br>
>> >> >> >> >> >> >> >> > moving<br>
>> >> >> >> >> >> >> >> > code)<br>
>> >> >> >> >> >> >> >> > to<br>
>> >> >> >> >> >> >> >> > fix<br>
>> >> >> >> >> >> >> >> > the<br>
>> >> >> >> >> >> >> >> > explicit case - it sounds like we now have two bits<br>
>> >> >> >> >> >> >> >> > of<br>
>> >> >> >> >> >> >> >> > code,<br>
>> >> >> >> >> >> >> >> > one<br>
>> >> >> >> >> >> >> >> > for<br>
>> >> >> >> >> >> >> >> > implicit and one for explicit - perhaps there's a<br>
>> >> >> >> >> >> >> >> > single<br>
>> >> >> >> >> >> >> >> > bit<br>
>> >> >> >> >> >> >> >> > of<br>
>> >> >> >> >> >> >> >> > code<br>
>> >> >> >> >> >> >> >> > that we<br>
>> >> >> >> >> >> >> >> > could write that would handle both?)<br>
>> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> The codegen paths are different -- otherwise as you<br>
>> >> >> >> >> >> >> >> commented,<br>
>> >> >> >> >> >> >> >> the<br>
>> >> >> >> >> >> >> >> implicit case would have been broken too.<br>
>> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> Refactoring FE code to handle both is probably beyond<br>
>> >> >> >> >> >> >> >> the<br>
>> >> >> >> >> >> >> >> scope<br>
>> >> >> >> >> >> >> >> of<br>
>> >> >> >> >> >> >> >> this fix.  Having a good test case here will exactly<br>
>> >> >> >> >> >> >> >> help<br>
>> >> >> >> >> >> >> >> avoid<br>
>> >> >> >> >> >> >> >> regression if that happens in the future.<br>
>> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> David<br>
>> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> > - David<br>
>> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> David<br>
>> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> > Or just any<br>
>> >> >> >> >> >> >> >> >> >> > compiler-generated ones? (you could drop<br>
>> >> >> >> >> >> >> >> >> >> > these<br>
>> >> >> >> >> >> >> >> >> >> > lines<br>
>> >> >> >> >> >> >> >> >> >> > if<br>
>> >> >> >> >> >> >> >> >> >> > it's<br>
>> >> >> >> >> >> >> >> >> >> > about<br>
>> >> >> >> >> >> >> >> >> >> > any<br>
>> >> >> >> >> >> >> >> >> >> > compiler-generated ones, might be<br>
>> >> >> >> >> >> >> >> >> >> > simpler/more<br>
>> >> >> >> >> >> >> >> >> >> > obvious<br>
>> >> >> >> >> >> >> >> >> >> > that<br>
>> >> >> >> >> >> >> >> >> >> > it's<br>
>> >> >> >> >> >> >> >> >> >> > not<br>
>> >> >> >> >> >> >> >> >> >> > about<br>
>> >> >> >> >> >> >> >> >> >> > the "= default" feature)<br>
>> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> Other compiler generated ones are handled<br>
>> >> >> >> >> >> >> >> >> >> differently.<br>
>> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> thanks,<br>
>> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> David<br>
>> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >> +  // PGOGEN: define {{.*}}@_ZN1AaSERKS_(<br>
>> >> >> >> >> >> >> >> >> >> >> +  // PGOGEN: %pgocount = load {{.*}}<br>
>> >> >> >> >> >> >> >> >> >> >> @__profc__ZN1AaSERKS_<br>
>> >> >> >> >> >> >> >> >> >> >> +  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1<br>
>> >> >> >> >> >> >> >> >> >> >> +  // PGOGEN:<br>
>> >> >> >> >> >> >> >> >> >> >> store{{.*}}@__profc__ZN1AaSERKS_<br>
>> >> >> >> >> >> >> >> >> >> >> +  A &operator=(A &&) = default;<br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >> +  // PGOGEN: define {{.*}}@_ZN1AaSEOS_<br>
>> >> >> >> >> >> >> >> >> >> >> +  // PGOGEN: %pgocount = load {{.*}}<br>
>> >> >> >> >> >> >> >> >> >> >> @__profc__ZN1AaSEOS_<br>
>> >> >> >> >> >> >> >> >> >> >> +  // PGOGEN: {{.*}}add{{.*}}%pgocount, 1<br>
>> >> >> >> >> >> >> >> >> >> >> +  // PGOGEN:<br>
>> >> >> >> >> >> >> >> >> >> >> store{{.*}}@__profc__ZN1AaSEOS_<br>
>> >> >> >> >> >> >> >> >> >> >> +<br>
>> >> >> >> >> >> >> >> >> >> >> +  // Check that coverage mapping includes 6<br>
>> >> >> >> >> >> >> >> >> >> >> function<br>
>> >> >> >> >> >> >> >> >> >> >> records<br>
>> >> >> >> >> >> >> >> >> >> >> including<br>
>> >> >> >> >> >> >> >> >> >> >> the<br>
>> >> >> >> >> >> >> >> >> >> >> +  // defaulted copy and move operators:<br>
>> >> >> >> >> >> >> >> >> >> >> A::operator=<br>
>> >> >> >> >> >> >> >> >> >> >> +  // COVMAP: @__llvm_coverage_mapping =<br>
>> >> >> >> >> >> >> >> >> >> >> {{.*}}<br>
>> >> >> >> >> >> >> >> >> >> >> {<br>
>> >> >> >> >> >> >> >> >> >> >> {<br>
>> >> >> >> >> >> >> >> >> >> >> i32,<br>
>> >> >> >> >> >> >> >> >> >> >> i32,<br>
>> >> >> >> >> >> >> >> >> >> >> i32,<br>
>> >> >> >> >> >> >> >> >> >> >> 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,<br>
>> >> >> >> >> >> >> >> >> >> > would<br>
>> >> >> >> >> >> >> >> >> >> > be<br>
>> >> >> >> >> >> >> >> >> >> > to<br>
>> >> >> >> >> >> >> >> >> >> > just<br>
>> >> >> >> >> >> >> >> >> >> > take<br>
>> >> >> >> >> >> >> >> >> >> > 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,<br>
>> >> >> >> >> >> >> >> >> >> > since<br>
>> >> >> >> >> >> >> >> >> >> > it's a<br>
>> >> >> >> >> >> >> >> >> >> > clang<br>
>> >> >> >> >> >> >> >> >> >> > code<br>
>> >> >> >> >> >> >> >> >> >> > change/fix.<br>
>> >> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >> +  return 0;<br>
>> >> >> >> >> >> >> >> >> >> >> +}<br>
>> >> >> >> >> >> >> >> >> >> >> Index: lib/CodeGen/CGClass.cpp<br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >> ===================================================================<br>
>> >> >> >> >> >> >> >> >> >> >> --- lib/CodeGen/CGClass.cpp<br>
>> >> >> >> >> >> >> >> >> >> >> +++ lib/CodeGen/CGClass.cpp<br>
>> >> >> >> >> >> >> >> >> >> >> @@ -1608,6 +1608,7 @@<br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >>    LexicalScope Scope(*this,<br>
>> >> >> >> >> >> >> >> >> >> >> RootCS->getSourceRange());<br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >> +  incrementProfileCounter(RootCS);<br>
>> >> >> >> >> >> >> >> >> >> >>    AssignmentMemcpyizer AM(*this, AssignOp,<br>
>> >> >> >> >> >> >> >> >> >> >> Args);<br>
>> >> >> >> >> >> >> >> >> >> >>    for (auto *I : RootCS->body())<br>
>> >> >> >> >> >> >> >> >> >> >>      AM.emitAssignment(I);<br>
>> >> >> >> >> >> >> >> >> >> >><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>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> >> >><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>
>> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> ><br>
>> >> >> >> >> >> ><br>
>> >> >> >> >> ><br>
>> >> >> >> >> ><br>
>> >> >> >> ><br>
>> >> >> >> ><br>
>> >> >> ><br>
>> >> >> ><br>
>> >> ><br>
>> >> ><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>