[PATCH] D16947: [PGO] assignment operator does not get profile data

Tue Feb 9 11:55:21 PST 2016

On Tue, Feb 9, 2016 at 11:44 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Tue, Feb 9, 2016 at 11:41 AM, Xinliang David Li <davidxl at google.com>
> wrote:
>>
>> On Tue, Feb 9, 2016 at 11:30 AM, David Blaikie <dblaikie at gmail.com> wrote:
>> >
>> >
>> > On Tue, Feb 9, 2016 at 11:26 AM, Xinliang David Li <davidxl at google.com>
>> > wrote:
>> >>
>> >> On Tue, Feb 9, 2016 at 11:14 AM, David Blaikie <dblaikie at gmail.com>
>> >> wrote:
>> >> >
>> >> >
>> >> > On Mon, Feb 8, 2016 at 9:23 PM, Xinliang David Li
>> >> > <davidxl at google.com>
>> >> > wrote:
>> >> >>
>> >> >> Wrong in the sense the the coverage result for the default operators
>> >> >> (the line where they are declared) is marked as if they are not
>> >> >> called
>> >> >> which can be confusing to the user.
>> >> >
>> >> >
>> >> > Presumably a user would have the same problem with implicit ops - the
>> >> > class
>> >> > header/name would be marked as if there was code that was not called
>> >> > there?
>> >>
>> >> that would be confusing though -- as data of many implicitly declared
>> >> functions will be lumped together and user won't know what that is .
>> >
>> >
>> > Presumably it's still going to be confusing, though - the line table
>> > will
>> > record that line and the counter won't be there, so the header of the
>> > type
>> > will be marked in red & a user worried about coverage may go through
>> > some
>> > contortions to try to discover and cover that code, no? (even though it
>> > may
>> > already be covered & is just being reported incorrectly due to their
>> > being
>> > no counters)
>>
>> coverage mapping does not use the debug line table for this purpose --
>> it encodes line info in source regions of its own format. Marking
>> class header as read will be super confusing if it happens (but does
>> not).
>
>
> 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?

This is subjective  -- but it is clear to me if a user explicitly
write a defaulted function, he/she would expect the function to be
covered in test. Class/struct tag on the hand, won't be expected to be
covered. By the way, we can do this one way or the other, but they
need to be consistent.

>
>>
>> Further discussion is welcome, but I am going to commit this change soon.
>
>
> As for the review - I'd still request that the test be in Clang, where the
> code change is.

Yes -- this patch only includes clang change.

 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.

I disagree. There are hundreds of such tests in compiler-rt, so I
don't see the point of holding off one more test case (or be a problem
for future batch porting).

David

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