On Tue, Feb 9, 2016 at 12:07 PM, David Li via llvm-commits <
llvm-comm...@lists.llvm.org> wrote:
> This revision was automatically updated to reflect the committed changes.
> Closed by commit rL260270: [PGO] Fix issue: explicitly defaulted assignop
> is not profiled (authored by davidxl).
>
In ge
dblaikie added inline comments.
Comment at: cfe/trunk/test/Profile/def-assignop.cpp:27
@@ +26,3 @@
+
+int main() {
+ A a1, a2;
This doesn't need to be main or have an int return. Just make it a void
function (with some generic name) & drop the "return 0" to keep
This revision was automatically updated to reflect the committed changes.
Closed by commit rL260270: [PGO] Fix issue: explicitly defaulted assignop is
not profiled (authored by davidxl).
Changed prior to commit:
http://reviews.llvm.org/D16947?vs=47239&id=47351#toc
Repository:
rL LLVM
http:/
On Tue, Feb 9, 2016 at 11:44 AM, David Blaikie wrote:
>
>
> On Tue, Feb 9, 2016 at 11:41 AM, Xinliang David Li
> wrote:
>>
>> On Tue, Feb 9, 2016 at 11:30 AM, David Blaikie wrote:
>> >
>> >
>> > On Tue, Feb 9, 2016 at 11:26 AM, Xinliang David Li
>> > wrote:
>> >>
>> >> On Tue, Feb 9, 2016 at 11
On Tue, Feb 9, 2016 at 11:41 AM, Xinliang David Li
wrote:
> On Tue, Feb 9, 2016 at 11:30 AM, David Blaikie wrote:
> >
> >
> > On Tue, Feb 9, 2016 at 11:26 AM, Xinliang David Li
> > wrote:
> >>
> >> On Tue, Feb 9, 2016 at 11:14 AM, David Blaikie
> wrote:
> >> >
> >> >
> >> > On Mon, Feb 8, 2016
On Tue, Feb 9, 2016 at 11:30 AM, David Blaikie wrote:
>
>
> On Tue, Feb 9, 2016 at 11:26 AM, Xinliang David Li
> wrote:
>>
>> On Tue, Feb 9, 2016 at 11:14 AM, David Blaikie wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 9:23 PM, Xinliang David Li
>> > wrote:
>> >>
>> >> Wrong in the sense the the
On Tue, Feb 9, 2016 at 11:26 AM, Xinliang David Li
wrote:
> On Tue, Feb 9, 2016 at 11:14 AM, David Blaikie wrote:
> >
> >
> > On Mon, Feb 8, 2016 at 9:23 PM, Xinliang David Li
> > wrote:
> >>
> >> Wrong in the sense the the coverage result for the default operators
> >> (the line where they are
On Tue, Feb 9, 2016 at 11:14 AM, David Blaikie wrote:
>
>
> On Mon, Feb 8, 2016 at 9:23 PM, Xinliang David Li
> 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
On Mon, Feb 8, 2016 at 9:23 PM, Xinliang David Li
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 impl
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.
David
On Mon, Feb 8, 2016 at 9:09 PM, David Blaikie wrote:
>
>
> On Mon, Feb 8, 2016 at 9:00 PM, Xinliang David Li
> wr
On Mon, Feb 8, 2016 at 9:00 PM, Xinliang David Li
wrote:
> On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie wrote:
> >
> > On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li
> > wrote:
> >>
> >> I took a look at the problem. The implicitly defaulted operators
> >> should not be instrumented as spec
On Mon, Feb 8, 2016 at 8:46 PM, David Blaikie wrote:
>
> On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li
> 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
On Mon, Feb 8, 2016 at 7:39 PM, Xinliang David Li
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 an
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 operato
On Mon, Feb 8, 2016 at 5:05 PM, Xinliang David Li
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 i
ha! somehow I kept thinking you are referring to implicit declared ctors.
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.
thanks,
David
On Mon, Feb 8, 2016
On Mon, Feb 8, 2016 at 4:31 PM, Xinliang David Li
wrote:
> On Mon, Feb 8, 2016 at 4:05 PM, David Blaikie wrote:
> >
> >
> > On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li
> > wrote:
> >>
> >> To be clear, you are suggesting breaking the test into two (one for
> >> copy, and one for move) ? I
On Mon, Feb 8, 2016 at 4:05 PM, David Blaikie wrote:
>
>
> On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li
> 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
On Mon, Feb 8, 2016 at 3:58 PM, Xinliang David Li
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
To be clear, you are suggesting breaking the test into two (one for
copy, and one for move) ? I am totally fine with that. 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.
thanks,
David
On Mon, Feb
On Mon, Feb 8, 2016 at 3:46 PM, Xinliang David Li
wrote:
> On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie wrote:
> >
> >
> > On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li
> > wrote:
> >>
> >> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie
> wrote:
> >> >
> >> >
> >> > On Mon, Feb 8, 2016 at
On Mon, Feb 8, 2016 at 3:35 PM, David Blaikie wrote:
>
>
> On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li
> wrote:
>>
>> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li
>> > wrote:
>> >>
>> >> On Mon, Feb 8, 2016 at 11:39
On Mon, Feb 8, 2016 at 3:21 PM, Xinliang David Li
wrote:
> On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie wrote:
> >
> >
> > On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li
> > wrote:
> >>
> >> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie
> wrote:
> >> >
> >> >
> >> > On Mon, Feb 8, 2016 a
On Mon, Feb 8, 2016 at 3:17 PM, David Blaikie wrote:
>
>
> On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li
> wrote:
>>
>> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie wrote:
>> >
>> >
>> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
>> > wrote:
>> >>
>> >> davidxl updated thi
On Mon, Feb 8, 2016 at 12:07 PM, Xinliang David Li
wrote:
> On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie wrote:
> >
> >
> > On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
> > wrote:
> >>
> >> davidxl updated this revision to Diff 47217.
> >> davidxl added a comment.
> >>
> >> Simpl
davidxl updated this revision to Diff 47239.
davidxl added a comment.
Further simplify tests according to David B's comment.
http://reviews.llvm.org/D16947
Files:
lib/CodeGen/CGClass.cpp
test/Profile/def-assignop.cpp
Index: test/Profile/def-assignop.cpp
On Mon, Feb 8, 2016 at 11:39 AM, David Blaikie wrote:
>
>
> On Mon, Feb 8, 2016 at 9:25 AM, David Li via llvm-commits
> wrote:
>>
>> davidxl updated this revision to Diff 47217.
>> davidxl added a comment.
>>
>> Simplified test case suggested by Vedant.
>>
>>
>> http://reviews.llvm.org/D16947
>>
ah, right, sorry about that - gmail didn't render cfe-commits on the to
line in the first email... weird.
Anyway, no need to include llvm-commits on clang-only changes.
On Mon, Feb 8, 2016 at 11:30 AM, Xinliang David Li
wrote:
> Both cfe-commits and llvm-commits are cc'ed.
>
> David
>
> On Mon,
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
> te
Both cfe-commits and llvm-commits are cc'ed.
David
On Mon, Feb 8, 2016 at 11:29 AM, David Blaikie wrote:
> This looks like a change to clang - could you test it in clang (& review it
> on cfe-commits instead of llvm-commits)?
>
> On Sat, Feb 6, 2016 at 11:57 AM, David Li via cfe-commits
> wrote
This looks like a change to clang - could you test it in clang (& review it
on cfe-commits instead of llvm-commits)?
On Sat, Feb 6, 2016 at 11:57 AM, David Li via cfe-commits <
cfe-commits@lists.llvm.org> wrote:
> davidxl created this revision.
> davidxl added a reviewer: vsk.
> davidxl added sub
vsk added a comment.
Lgtm.
http://reviews.llvm.org/D16947
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
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
=
vsk added a comment.
Thanks! This is definitely the right fix, since it fixes up
implicitly-generated move constructors as well. However, I think the test can
be simpler (while also checking that move constructors are handled correctly).
E.g:
struct A {
void operator=(const A &a) {}
davidxl updated this revision to Diff 47150.
davidxl added a comment.
Updated test case.
Another test case in profile-rt (pending) is : http://reviews.llvm.org/D16974
http://reviews.llvm.org/D16947
Files:
lib/CodeGen/CGClass.cpp
test/Profile/def-assignop.cpp
Index: test/Profile/def-assign
davidxl created this revision.
davidxl added a reviewer: vsk.
davidxl added subscribers: llvm-commits, cfe-commits.
For compiler generated assignment operator that is not trivial (calling base
class operator=()), Clang FE assign region counters to the function body but
does not emit profile coun
36 matches
Mail list logo
