<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Feb 9, 2016 at 11:55 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class=""><div class="h5">On Tue, Feb 9, 2016 at 11:44 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
> On Tue, Feb 9, 2016 at 11:41 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: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>><br>
>> >> wrote:<br>
>> >> ><br>
>> >> ><br>
>> >> > On Mon, Feb 8, 2016 at 9:23 PM, Xinliang David Li<br>
>> >> > <<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<br>
>> >> >> 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<br>
>> > will<br>
>> > record that line and the counter won't be there, so the header of the<br>
>> > type<br>
>> > will be marked in red & a user worried about coverage may go through<br>
>> > some<br>
>> > contortions to try to discover and cover that code, no? (even though it<br>
>> > may<br>
>> > already be covered & is just being reported incorrectly due to their<br>
>> > being<br>
>> > no counters)<br>
>><br>
>> 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>
><br>
><br>
> Then why would it be a problem to omit the defaulted versions? It won't be<br>
> marked "as if they are not called" (it won't show up red) it'll just be<br>
> marked as if there's no code there, right?<br>
<br>
</div></div>This is subjective  -- but it is clear to me if a user explicitly<br>
write a defaulted function, he/she would expect the function to be<br>
covered in test. </blockquote><div><br></div><div>I think that might be a misunderstanding of the reasons/situations in which one might write a defaulted op.<br><br>Generally we encourage users to follow the rule of zero ( <a href="http://en.cppreference.com/w/cpp/language/rule_of_three">http://en.cppreference.com/w/cpp/language/rule_of_three</a> ) but the most common situation I've seen = default used is when access needs to be changed (eg: the default would be public but for some reason it needs to be protected) or where the default is not available (eg: you needed to make the dtor private, but you wanted your move ops to be public).<br><br>I don't think there's necessarily much correlation between whether something should be covered/tested and whether it's implicit or explicit.<br><br>Perhaps triviality is a relevant/better test? (which is, more or less "could this operation me a memcpy, or does it have to execute arbitrary user code"?) Though even then I wouldn't be sure.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Class/struct tag on the hand, won't be expected to be<br>
covered. By the way, we can do this one way or the other, but they<br>
need to be consistent.<br>
<span class=""><br>
><br>
>><br>
>> Further discussion is welcome, but I am going to commit this change soon.<br>
><br>
><br>
> As for the review - I'd still request that the test be in Clang, where the<br>
> code change is.<br>
<br>
</span>Yes -- this patch only includes clang change.<br></blockquote><div><br></div><div>Ah, so I see. Thanks for that.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<span class=""><br>
 Please defer adding tests like this to compiler-rt until<br>
> we've had some discussion with compiler-rt folks (I tried to rope Alexey<br>
> into one of these threads, not sure if it got lost in the noise), perhaps on<br>
> llvm-dev. Maybe I'm confused about what testing should go in compiler-rt,<br>
> but we can defer that to a discussion as there should be a test in Clang<br>
> regardless of whether there's also testing for this specific feature in<br>
> compiler-rt.<br>
<br>
</span>I disagree. There are hundreds of such tests in compiler-rt, so I<br>
don't see the point of holding off one more test case (or be a problem<br>
for future batch porting).<br>
<span class=""><font color="#888888"><br>
David<br>
</font></span><div class=""><div class="h5"><br>
<br>
<br>
><br>
> - David<br>
><br>
>><br>
>><br>
>> thanks,<br>
>><br>
>> David<br>
>><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<br>
>> >> >> >> <<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<br>
>> >> >> >> >> 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<br>
>> >> >> >> >> generated)<br>
>> >> >> >> >> right<br>
>> >> >> >> >> after my previous reply and it still passes with this patch.<br>
>> >> >> >> >> 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<br>
>> >> >> >> >> implicit<br>
>> >> >> >> >> bit<br>
>> >> >> >> >> set, step 1) is skipped as designed, so step 2) simply becomes<br>
>> >> >> >> >> a<br>
>> >> >> >> >> no-op.<br>
>> >> >> >> >><br>
>> >> >> >> >> In short, the test case still needs explicit '=default', and<br>
>> >> >> >> >> 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<br>
>> >> >> >> instrumentation<br>
>> >> >> >> overhead. They are not needed for coveraging test either (as<br>
>> >> >> >> there<br>
>> >> >> >> are<br>
>> >> >> >> no source lines to cover). This is a good tuning heuristic in<br>
>> >> >> >> most<br>
>> >> >> >> cases, but can get wrong sometimes (IR based late instrumentation<br>
>> >> >> >> 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<br>
>> >> >> >> >> >> 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<br>
>> >> >> >> >> > 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" -<br>
>> >> >> >> >> > I<br>
>> >> >> >> >> > don't<br>
>> >> >> >> >> > think<br>
>> >> >> >> >> > there's value in testing both the explicit default and<br>
>> >> >> >> >> > implicit<br>
>> >> >> >> >> > default<br>
>> >> >> >> >> > if<br>
>> >> >> >> >> > it's just the default-y-ness that's relevant here. Otherwise<br>
>> >> >> >> >> > 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<br>
>> >> >> >> >> > 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<br>
>> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> >> into<br>
>> >> >> >> >> >> >> >> two<br>
>> >> >> >> >> >> >> >> (one<br>
>> >> >> >> >> >> >> >> for<br>
>> >> >> >> >> >> >> >> copy, and one for move) ? I am totally fine with<br>
>> >> >> >> >> >> >> >> that.<br>
>> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> ><br>
>> >> >> >> >> >> >> > Nah, no need to split the test case - we try to keep<br>
>> >> >> >> >> >> >> > the<br>
>> >> >> >> >> >> >> > number<br>
>> >> >> >> >> >> >> > of<br>
>> >> >> >> >> >> >> > test<br>
>> >> >> >> >> >> >> > files down (& group related tests into a single file)<br>
>> >> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> >> because<br>
>> >> >> >> >> >> >> >> they<br>
>> >> >> >> >> >> >> >> might<br>
>> >> >> >> >> >> >> >> share the same code path (clang's implementation) as<br>
>> >> >> >> >> >> >> >> 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<br>
>> >> >> >> >> >> >> > 5/many<br>
>> >> >> >> >> >> >> > more<br>
>> >> >> >> >> >> >> > things<br>
>> >> >> >> >> >> >> > that<br>
>> >> >> >> >> >> >> > shared the same codepath, I'd generally suggest<br>
>> >> >> >> >> >> >> > testing a<br>
>> >> >> >> >> >> >> > representative<br>
>> >> >> >> >> >> >> > sample (possibly just a single one) rather than<br>
>> >> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> > we're<br>
>> >> >> >> >> >> >> > talking<br>
>> >> >> >> >> >> >> > about<br>
>> >> >> >> >> >> >> > test<br>
>> >> >> >> >> >> >> > granularity, it's probably worth just putting these<br>
>> >> >> >> >> >> >> > 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).<br>
>> >> >> >> >> >> >> However<br>
>> >> >> >> >> >> >> 2) having too many cases grouped together also reduces<br>
>> >> >> >> >> >> >> 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<br>
>> >> >> >> >> >> > spectacularly<br>
>> >> >> >> >> >> > (when<br>
>> >> >> >> >> >> > you<br>
>> >> >> >> >> >> > can't<br>
>> >> >> >> >> >> > order the IR emission to happen mostly in the order that<br>
>> >> >> >> >> >> > the<br>
>> >> >> >> >> >> > source<br>
>> >> >> >> >> >> > is<br>
>> >> >> >> >> >> > written (rather being interleaved))<br>
>> >> >> >> >> >> ><br>
>> >> >> >> >> >> > Anyway, up to you - that part isn't something I'm<br>
>> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> > and<br>
>> >> >> >> >> >> >> > explicitly<br>
>> >> >> >> >> >> >> > defaulted ops.<br>
>> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> I may take a look at that when I find time -- but there<br>
>> >> >> >> >> >> >> 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<br>
>> >> >> >> >> >> > the<br>
>> >> >> >> >> >> > codepath<br>
>> >> >> >> >> >> > you<br>
>> >> >> >> >> >> > changed is used for both the explicitly (based on the<br>
>> >> >> >> >> >> > change<br>
>> >> >> >> >> >> > fixing<br>
>> >> >> >> >> >> > your<br>
>> >> >> >> >> >> > test case) and implicitly defaulted (based on my test<br>
>> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> >> >> >> >> 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<br>
>> >> >> >> >> >> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> >> >> >> >> >> 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>
>> >> >> >> >> >> >> >> >> >> >> >> ===================================================================<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<br>
>> >> >> >> >> >> >> >> >> >> >> 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<br>
>> >> >> >> >> >> >> >> >> >> >> 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<br>
>> >> >> >> >> >> >> >> >> >> > multiple<br>
>> >> >> >> >> >> >> >> >> >> > instances<br>
>> >> >> >> >> >> >> >> >> >> > of<br>
>> >> >> >> >> >> >> >> >> >> > the<br>
>> >> >> >> >> >> >> >> >> >> > same<br>
>> >> >> >> >> >> >> >> >> >> > codepath (so the copy and move ctor for<br>
>> >> >> >> >> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> >> >> >> 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,<br>
>> >> >> >> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> >> > 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,<br>
>> >> >> >> >> >> >> >> >> > for<br>
>> >> >> >> >> >> >> >> >> > example -<br>
>> >> >> >> >> >> >> >> >> > we<br>
>> >> >> >> >> >> >> >> >> > test<br>
>> >> >> >> >> >> >> >> >> > each<br>
>> >> >> >> >> >> >> >> >> > optimization in isolation. The same would be<br>
>> >> >> >> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> >> >> 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<br>
>> >> >> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> >> >> >> >> 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<br>
>> >> >> >> >> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> >> >> > than<br>
>> >> >> >> >> >> >> >> >> > moving<br>
>> >> >> >> >> >> >> >> >> > code)<br>
>> >> >> >> >> >> >> >> >> > to<br>
>> >> >> >> >> >> >> >> >> > fix<br>
>> >> >> >> >> >> >> >> >> > the<br>
>> >> >> >> >> >> >> >> >> > explicit case - it sounds like we now have two<br>
>> >> >> >> >> >> >> >> >> > bits<br>
>> >> >> >> >> >> >> >> >> > of<br>
>> >> >> >> >> >> >> >> >> > code,<br>
>> >> >> >> >> >> >> >> >> > one<br>
>> >> >> >> >> >> >> >> >> > for<br>
>> >> >> >> >> >> >> >> >> > implicit and one for explicit - perhaps there's<br>
>> >> >> >> >> >> >> >> >> > 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<br>
>> >> >> >> >> >> >> >> >> you<br>
>> >> >> >> >> >> >> >> >> commented,<br>
>> >> >> >> >> >> >> >> >> the<br>
>> >> >> >> >> >> >> >> >> implicit case would have been broken too.<br>
>> >> >> >> >> >> >> >> >><br>
>> >> >> >> >> >> >> >> >> Refactoring FE code to handle both is probably<br>
>> >> >> >> >> >> >> >> >> beyond<br>
>> >> >> >> >> >> >> >> >> the<br>
>> >> >> >> >> >> >> >> >> scope<br>
>> >> >> >> >> >> >> >> >> of<br>
>> >> >> >> >> >> >> >> >> this fix.  Having a good test case here will<br>
>> >> >> >> >> >> >> >> >> 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<br>
>> >> >> >> >> >> >> >> >> >> >> >> 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<br>
>> >> >> >> >> >> >> >> >> >> >> > 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>
>> >> >> >> >> >> >> >> >> >> >> >> ===================================================================<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,<br>
>> >> >> >> >> >> >> >> >> >> >> >> AssignOp,<br>
>> >> >> >> >> >> >> >> >> >> >> >> Args);<br>
>> >> >> >> >> >> >> >> >> >> >> >>    for (auto *I : RootCS->body())<br>
>> >> >> >> >> >> >> >> >> >> >> >>      AM.emitAssignment(I);<br>
>> >> >> >> >> >> >> >> >> >> >> >><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>
>> >> >> >> >> >> >> >> >> >> >> >><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>
><br>
><br>
</div></div></blockquote></div><br></div></div>