On Tue, Feb 9, 2016 at 11:26 AM, Xinliang David Li <davi...@google.com> wrote:
> On Tue, Feb 9, 2016 at 11:14 AM, David Blaikie <dblai...@gmail.com> wrote: > > > > > > On Mon, Feb 8, 2016 at 9:23 PM, Xinliang David Li <davi...@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) > > David > > > > > - David > > > >> > >> > >> David > >> > >> On Mon, Feb 8, 2016 at 9:09 PM, David Blaikie <dblai...@gmail.com> > wrote: > >> > > >> > > >> > On Mon, Feb 8, 2016 at 9:00 PM, Xinliang David Li <davi...@google.com > > > >> > wrote: > >> >> > >> >> On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie <dblai...@gmail.com> > >> >> wrote: > >> >> > > >> >> > On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li > >> >> > <davi...@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 <dblai...@gmail.com > > > >> >> >> wrote: > >> >> >> > > >> >> >> > > >> >> >> > On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li > >> >> >> > <davi...@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 > >> >> >> >> <dblai...@gmail.com> > >> >> >> >> wrote: > >> >> >> >> > > >> >> >> >> > > >> >> >> >> > On Mon, Feb 8, 2016 at 4:31 PM, Xinliang David Li > >> >> >> >> > <davi...@google.com> > >> >> >> >> > wrote: > >> >> >> >> >> > >> >> >> >> >> On Mon, Feb 8, 2016 at 4:05 PM, David Blaikie > >> >> >> >> >> <dblai...@gmail.com> > >> >> >> >> >> wrote: > >> >> >> >> >> > > >> >> >> >> >> > > >> >> >> >> >> > On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li > >> >> >> >> >> > <davi...@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 > >> >> >> >> >> >> <dblai...@gmail.com> > >> >> >> >> >> >> wrote: > >> >> >> >> >> >> > > >> >> >> >> >> >> > > >> >> >> >> >> >> > On Mon, Feb 8, 2016 at 3:46 PM, Xinliang David Li > >> >> >> >> >> >> > <davi...@google.com> > >> >> >> >> >> >> > wrote: > >> >> >> >> >> >> >> > >> >> >> >> >> >> >> On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie > >> >> >> >> >> >> >> <dblai...@gmail.com> > >> >> >> >> >> >> >> wrote: > >> >> >> >> >> >> >> > > >> >> >> >> >> >> >> > > >> >> >> >> >> >> >> > On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li > >> >> >> >> >> >> >> > <davi...@google.com> > >> >> >> >> >> >> >> > wrote: > >> >> >> >> >> >> >> >> > >> >> >> >> >> >> >> >> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie > >> >> >> >> >> >> >> >> <dblai...@gmail.com> > >> >> >> >> >> >> >> >> wrote: > >> >> >> >> >> >> >> >> > > >> >> >> >> >> >> >> >> > > >> >> >> >> >> >> >> >> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David > Li > >> >> >> >> >> >> >> >> > <davi...@google.com> > >> >> >> >> >> >> >> >> > wrote: > >> >> >> >> >> >> >> >> >> > >> >> >> >> >> >> >> >> >> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie > >> >> >> >> >> >> >> >> >> <dblai...@gmail.com> > >> >> >> >> >> >> >> >> >> wrote: > >> >> >> >> >> >> >> >> >> > > >> >> >> >> >> >> >> >> >> > > >> >> >> >> >> >> >> >> >> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via > >> >> >> >> >> >> >> >> >> > llvm-commits > >> >> >> >> >> >> >> >> >> > <llvm-comm...@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-comm...@lists.llvm.org > >> >> >> >> >> >> >> >> >> >> > >> >> >> >> >> >> >> >> >> >> > >> >> >> >> >> >> >> >> >> >> > >> >> >> >> >> >> >> >> >> >> > >> >> >> >> >> >> >> >> >> >> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits > >> >> >> >> >> >> >> >> >> >> > >> >> >> >> >> >> >> >> >> > > >> >> >> >> >> >> >> >> > > >> >> >> >> >> >> >> >> > > >> >> >> >> >> >> >> > > >> >> >> >> >> >> >> > > >> >> >> >> >> >> > > >> >> >> >> >> >> > > >> >> >> >> >> > > >> >> >> >> >> > > >> >> >> >> > > >> >> >> >> > > >> >> >> > > >> >> >> > > >> >> > > >> >> > > >> > > >> > > > > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits