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

[David Blaikie via cfe-commits]

On Tue, Feb 9, 2016 at 11:55 AM, Xinliang David Li <davidxl at google.com>
wrote:

> 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.


I think that might be a misunderstanding of the reasons/situations in which
one might write a defaulted op.

Generally we encourage users to follow the rule of zero (
http://en.cppreference.com/w/cpp/language/rule_of_three ) 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).

I don't think there's necessarily much correlation between whether
something should be covered/tested and whether it's implicit or explicit.

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.


> 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.
>

Ah, so I see. Thanks for that.


>
>  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
> >> >> >> >> >> >> >> >> >> >> >> >>
> >> >> >> >> >> >> >> >> >> >> >> >
> >> >> >> >> >> >> >> >> >> >> >
> >> >> >> >> >> >> >> >> >> >> >
> >> >> >> >> >> >> >> >> >> >
> >> >> >> >> >> >> >> >> >> >
> >> >> >> >> >> >> >> >> >
> >> >> >> >> >> >> >> >> >
> >> >> >> >> >> >> >> >
> >> >> >> >> >> >> >> >
> >> >> >> >> >> >> >
> >> >> >> >> >> >> >
> >> >> >> >> >> >
> >> >> >> >> >> >
> >> >> >> >> >
> >> >> >> >> >
> >> >> >> >
> >> >> >> >
> >> >> >
> >> >> >
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160209/f1485e63/attachment-0001.html>