- User Since
- Oct 19 2012, 12:57 AM (244 w, 21 h)
Tue, Jun 6
@srhines: Do you need Android to *always* be 64-bit, even if someone forces aapcs with it?
I don't see why we should force the target, given that this is a CodeGen test, should worr (and be tested) on all targets.
Just FYI, when you submit a patch, use "git diff -U999" so that we can see the context in the web interface. Makes it easier to review. :)
Where is this failing?
Mon, Jun 5
Fri, Jun 2
Makes sense to me. But now that "generic" is the default, it'll impact all cores equally, so I'll let other people comment before approving.
Wed, May 31
Tue, May 30
A few more comments... :)
Fri, May 26
Hi, can you please re-share the patch with more context?
Thu, May 25
Silly inline comment, but otherwise, LGTM. Thanks!
May 24 2017
May 23 2017
Ok, I think it's good as it is. Looking forward to more patches in this area. :)
Approving, so that when Steve does the same, the review is free to commit.
May 22 2017
Some initial comments...
Wait, Stephen had a concern, we need his feedback on my reply before approving.
Hi Ayal, sorry for the delay. Can you commit the plan doc separately?
May 21 2017
Just to be clear, I'm not *against* the idea of an intrinsic, nor I'm pushing this patch for any personal/professional agenda. I hope I have made that perfectly clear on my previous reviews on the same patches before.
May 20 2017
Right, I think this is a useful thing to have, but it needs a better plan.
May 19 2017
May 18 2017
May 17 2017
May 16 2017
May 13 2017
May 12 2017
Right, just to make sure it's doing what we expect it to, did you try to run this with targets "armv7a" and "armv4t"? They should have different results.
May 11 2017
May 10 2017
As parsers go, simplicity and elegance are usually mutually exclusive.
May 9 2017
Wait, we also need the TTI callbacks. I thought you were going to introduce them.
May 8 2017
Right, this is looking much better. Now, what about tests?
May 5 2017
Also, where would this be used?
Sorry for the delay, LGTM. Thanks!
@echristo, any comments on this?
May 4 2017
Right, makes sense. This is an obvious fix, so I feel comfortable approving this one. LGTM. Thanks!
Wouldn't it be better to set the type manually to something like uint32_t or uint64_t? To make sure they're the same across all arches?
I'm ok with the change, as long as @javed.absar is happy. :)
May 3 2017
I really thought it was there already! Thanks for the patch! LGTM.
You seem to have lost the negative test.
With that in mind, I think you should then go back to your original implementation (ignore quotes) and let the expression evaluation crash if there's any single quote in between (or whatever is the expected behaviour).
May 2 2017
May 1 2017
Thanks! LGTM with the test change.
Can you merge D32523 with this one?
Apr 28 2017
Right, I clicked the link and it took me to the old code. :)
Apr 27 2017
So, I think the document is so good and can be very helpful in further communications, that I think it should be committed separately.
Apr 26 2017
Hi Graham, thanks for this work.
I think this patch and D32523 are small and simple enough that it could be merged into one...