- User Since
- Jul 7 2012, 2:54 PM (242 w, 12 h)
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.)
Tue, Feb 21
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.
Fri, Feb 17
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.
Thu, Feb 16
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.
Wed, Feb 15
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.
Tue, Feb 14
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!
Mon, Feb 13
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... =[
Sat, Feb 11
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.
Fri, Feb 10
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.
Thu, Feb 9
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.
Thanks all, landing!
Wed, Feb 8
Nice catch and nice test case!
Rebase now that we actually don't have grep testing here.
Rebase and address a comment from Davide.
Mon, Feb 6
Yes, sorry. I thought I had abandoned all of the ones superseded but I missed this one.
Another update to actually remove all the support variables for maintaining an
in-flight DFS walk from the class. Somehow forgot to remove these, they're all
And this time with all the crazy super-linear assertions disabled outside of
"expensive checks" mode.
Marking that this is dead. I'll be sending out a fresh series of patches that take us in a different (and IMO much better) direction for argpromote...
Rebase and address review comments.
Thanks, comments hopefully addressed and new patch incoming.
Fri, Feb 3
Update to base on plain master where it can be submitted and remove discussion
of ArgPromote. Also updated to fix a scaling issue by putting some very
expensive asserts behind the appropriate macro guard.