- User Since
- Jul 7 2012, 2:54 PM (245 w, 5 d)
The fix seems trivial and looks good. Please submit to at least fix the crasher.
But we use this to send color codes to temporary files that capture output for build systems and such when -fcolor-diagnostics is explicitly provided? (As an example...)
Tue, Mar 21
This seems like a totally nice direction, but I wonder if I could convince you to scope creep this a bit?
Fri, Mar 17
Update with fixes suggested in review.
Function argument order fixed.
I think it'd be good to send a quick RFC / heads up email to the dev list about the refactorings you're planning here just so that folks know what the roadmap is. I'll review this in the mean time.
Thu, Mar 16
LGTM with whatever you decide about the allocator stuff.
LGTM once split.
This seems like a very reasonable tradeoff and a clean patch. Especially reasonable given the subsequent improvements. Ship it.
Sun, Mar 12
Fri, Mar 10
Nice, I like it.
Currently, I really don't understand why this is the right approach...
Update to fix subtle bugs in handling arguments. Thanks to David for the review
comments that caused me to see the issue here.
Thu, Mar 9
Wed, Mar 8
Tue, Mar 7
So, this didn't go to the list. I'd abandon it and create a fresh revision with llvm-commits CC'ed directly.
Mon, Mar 6
In addition to the issue David pointed out, I don't understand the motivation yet.
We intentionally didn't use zero because that seems more of a degenerate test case. What problem are you actually trying to solve here?
Fri, Mar 3
Wed, Mar 1
Tue, Feb 28
Mon, Feb 27
LGTM as well. Lemme know if you'd like me to land.
Sun, Feb 26
Fri, Feb 24
Some initial (minor) comments. It'd also help I think to rebase this as some of the refactorings land.
There's nothing wrong with this code, but this is a refactoring that I think only makes sense in the context where you need it. As-is, it seems like unnecessarily more layers with no real benefit. Let's look at this in the end-state patch?
I'm fine if you want this in the main patch, but I actually think this makes sense as a stand-alone refactoring, so LGTM.
Wed, Feb 22
(FWIW, I like the more minimal schemes here. We have a couple of sequential ones, and a solid divergent ones. Enough to be very useful, but no more than we really have use cases for today. That part LGTM, but leaving the code review proper to others.)
Feb 21 2017
Some longer term comments here, but all of this seems reasonably in-line with how we currently do things in reassociate, but raises some long-standing issues in a potentially more concerning area.
Feb 17 2017
LGTM, thanks for all the analysis!
FWIW, this looks fine to me with minor tweaks below. Would be great to have @MatzeB be happy as well though, this is a part of the backend I'm somewhat less familiar with.
LGTM, really nice cleanup.
Feb 16 2017
Can you split this into separate patches that do a single refactoring? (Especially considering you have the breakdown in your descirption...) That would make it easier to review for me.
LGTM with a tiny nit pick. I'll apply the fix and submit momentarily, but mentioning it here just for posterity.
Feb 15 2017
LGTM, this seems to be an obvious fix to the bug, and exactly matches the intent from the original patch.
Sorry to jump in with a peanut gallery comment, but just wanted to push back on some of the complexity we're adding here.
Feb 14 2017
I would still mention that deleting them is fine.
This at least seems like a step in the right direction, so LGTM. Maybe touch base with @majnemer before landing though in case he sees something I don't.
LGTM, sorry I didn't look right after you updated it!
Feb 13 2017
Just to be explicit, I agree with Hal's summary. This seems like the right engineering tradeoff and I don't find anything particularly unsatisfying about it.
So, we have a system that does *not* handle spaces in paths well. I would really like to continue to use the more restrictive rules for lit test names.
I don't find the code any more clear after this change, and I don't really see a realistic way future authors could (or should) continue to adhere to this kind of rule. I'm pretty strongly against contorting the code to pacify these kinds of messages... =[
Feb 11 2017
The basic reasoning makes sense to me when we do the binary search lowering. However, while you talk about simplify-cfg handling the lookup table case, this code should also respond well to the case where it will be lowered as a jump table.
Feb 10 2017
FWIW, this all looks good on my end. David, does this address your concern?
LGTM with a minor tweak above, feel free to just submit with that change unless you'd like to do something different.
Feb 9 2017
Cool! More detailed comments than last time, some of which is just teaching you some of the basics of modern test hygene here. Some high level comments:
Haven't made it all the way through, but I think it would be helpful to apply some of the comments so far across all the changes here.
This seems good to me. Check with Eric to see if we try to test this any any specific way?
Sorry, I really think you want Lang or someone more familiar with EE to review this...
Confirmed with Davide that this LGTM now on IRC, landing.