- User Since
- Oct 19 2012, 12:57 AM (239 w, 5 d)
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.
Mon, May 22
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?
Sun, May 21
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.
Sat, May 20
Right, I think this is a useful thing to have, but it needs a better plan.
Fri, May 19
Thu, May 18
Wed, May 17
Tue, May 16
Sat, May 13
Fri, May 12
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.
Thu, May 11
Wed, May 10
As parsers go, simplicity and elegance are usually mutually exclusive.
Tue, May 9
Wait, we also need the TTI callbacks. I thought you were going to introduce them.
Mon, May 8
Right, this is looking much better. Now, what about tests?
Fri, May 5
Also, where would this be used?
Sorry for the delay, LGTM. Thanks!
@echristo, any comments on this?
Thu, May 4
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. :)
Wed, May 3
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).
Tue, May 2
Mon, May 1
Thanks! LGTM with the test change.
Can you merge D32523 with this one?
Fri, Apr 28
Right, I clicked the link and it took me to the old code. :)
Thu, Apr 27
So, I think the document is so good and can be very helpful in further communications, that I think it should be committed separately.
Wed, Apr 26
Hi Graham, thanks for this work.
I think this patch and D32523 are small and simple enough that it could be merged into one...
Adding Tim, as this is likely going to affect Darwin more than Linux.
As soon as the dependency patch is approved, this one looks ok.
Well, for now, this isn't doing anything other than silently ignoring the directives.
Tue, Apr 25
This could have side-effects, too, especially when going through textual IR, no?
Mon, Apr 24
I'm not sure this will work at all. Not because it doesn't make sense (it does), but because of several bugs in the soft vs. hard float implementation in the Triple related classes all the way to the back-end.
We have some unit tests for Triple. Can you add some, pls?
Apr 23 2017
Apr 21 2017
Ok, I see no harm in letting you work upstream, since no one should be relying on the correct behaviour on opensuse for now. LGTM. Thanks!
Nothing new here, pretty much standard. No "gnueabihf"?
I agree with James, refactoring OptionalDef is a task too big for this patch.