- User Since
- Jul 7 2012, 2:54 PM (345 w, 3 d)
LGTM (with a minor cleanup, but feel free to just submit w/ that if we're all agreeing about this direction).
I think we can consider tweaking the generic ones separately.
Based on the -dev discussion, update once the target machine differences are addressed by mimicing the way the legacy PM works, which will hopefully restrict this similarly to what Eli is suggesting as well...
Mon, Feb 18
What about using /*/ at the ignore pattern? This allows top-level files, and makes only new top-level *directories* require an ignore update. To my mind, that seems a bit more narrowly scoped and might be a bit less surprising. Thoughts?
Tue, Feb 12
Thanks for seeing through this .... very non-trivial effort! =D Also, as I'm sure you know by now, will need to watch the bots carefully. Sadly, some may fall over.
Mon, Feb 11
Hey Marshall and Michael,
Sun, Feb 10
Fri, Feb 8
Thu, Feb 7
Sorry this completely fell off my radar. It is back on my radar, and I'll try to get to it in the next day or two.
Wed, Feb 6
This is looking really close. I think we should get some basic sanity checks w/ the kernel, and even if there is an obscure bug or two, land this and move to follow-up patches.
I think you may be able to do this in a slightly nicer way.
Tue, Feb 5
Mon, Feb 4
Just think we can poke the map a bit better here.
A few minor comments.
Minor past-commit nit.
Fri, Feb 1
Wed, Jan 30
(also LGTM with the updates above and any renaming you want to do)
Minor nits. I've been using Call instead of CB because when there is a nice word instead of initialism, I prefer it for readability. But that's up to you really.
There was a long discussion between two NetBSD maintainers about this (both already in the reviewers list of this patch). I'm not sure if there is an existing thread that would be better to follow up on as opposed to starting a fresh thread.
Tue, Jan 29
Made it through the optimizer code. Really minor changes here, I think this is looking good. Probable the biggest question marks are in the MI representation.
Looking at updated patch and some of the optimizer bits, but some inline responses to discussion points below:
Sorry it took so long, but made a full pass through this. Just want to point out that this patch is already in really great shape due to the huge amount of work from the original author, Craig taking it over, and the multiple rounds of review from Bill and Nick among others.
Mon, Jan 28
Fri, Jan 25
FWIW, I think this patch is trivially good when D57199 lands. I might suggest just merge this into that patch, but if you want to keep them separate, LGTM.
(peanut gallery comments)
Overall, really nice patch. A bunch of super nit-picky comments below, not much more to add beyond Sanjoy's review honestly. =D With these comments addressed, LGTM, but maybe double check if Sanjoy is happy w/ the update before submitting.
LGTM (provided I've not missed anything below w.r.t. atomics).
Wed, Jan 23
I live in fear of the use cases we haven't yet thought of for this that we will discover in the future. ;] But proceed, LGTM!