Gotcha. Going forward, if I feel anything requires a heads up, I'll do the full pre-commit review.
Thanks for the feedback, Mike On Mon, Jun 13, 2016 at 11:30 AM, Aaron Ballman <aaron.ball...@gmail.com> wrote: > On Mon, Jun 13, 2016 at 12:27 PM, Zachary Turner <ztur...@google.com> > wrote: > > I agree that it can be annoying to say "hey guys, i would normally do > post > > commit review on this, but i wanted to give the courtesy of a heads up", > and > > then potentially waiting an indeterminate amount of time. > > > > I think that actually discourages these kind of changes going up at all, > > because people will just say "well that's easy i just won't give the > heads > > up then", which i think would be a net loss > > That's why my preference is to give the heads up and do a full review. > Is it slower and more process-oriented? Yes. But so is all pre-commit > review. This is what keeps the quality standard we want. :-) > > ~Aaron > > > > > On Mon, Jun 13, 2016 at 9:10 AM Aaron Ballman <aaron.ball...@gmail.com> > > wrote: > >> > >> On Mon, Jun 13, 2016 at 11:55 AM, Michael Spertus <m...@spertus.com> > >> wrote: > >> > Hi David, > >> > While I understand the initial reasoning. I have found that this is > like > >> > a > >> > hundred times better for working on Clang in practice and can't > imagine > >> > working without it. The point is that many Clang data structures > contain > >> > SmallVectors and having to do zero expansion clicks instead of > multiple > >> > each > >> > time you take a step through the code is really helpful. If you want > me > >> > to > >> > back it out and rereview we can, but I'd encourage you to try it out > >> > first. > >> > >> Yeah, SmallVectors are somewhat click-heavy in MSVC currently. I've > >> not had the chance to try this patch out on anything practical, but it > >> seems like it is an improvement from what I've seen. > >> > >> > To ask more about the aside, I'm sorry if I violated community norms. > >> > Let me > >> > tell you my reasoning, and you can clarify how I should handle in the > >> > future: Aaron approved me to do post-commit reviews on natvis changes, > >> > which > >> > I have done frequently. For this change, I wasn't putting it into > >> > phabricator because I thought pre-commit approval is required but more > >> > as a > >> > heads up. Should I change that to be if I don't feel comfortable > >> > submitting > >> > without phabricator, then do the full review process? > >> > >> When you want to give the community a heads up on something, putting > >> it into phab (or starting an RFC thread on the mailing list) is a good > >> choice. However, when you start a patch in phab, it's good form to > >> wait for a reviewer to sign off before committing even if you could > >> also handle it with post-commit review. I'm not too worried about this > >> change, so I'm not suggesting it should be backed out. > >> > >> ~Aaron > >> > >> > > >> > Thanks, > >> > > >> > Mike > >> > > >> > On Mon, Jun 13, 2016 at 10:16 AM, David Blaikie <dblai...@gmail.com> > >> > wrote: > >> >> > >> >> As for the original change proposed: My guiding principle would be > "do > >> >> whatever std::vector does". (& that's what I did when implementing > GDB > >> >> pretty printers for SmallVector/SmallString/ArrayRef, etc... ) > >> >> > >> >> An aside: We generally don't do time limited reviews like this. > Either > >> >> something needs review because you're not sure about it, or it > doesn't. > >> >> It > >> >> sounds like the feedback you were looking for probably would've been > >> >> fine a > >> >> post-commit review feedback just as easily & perhaps might've been a > >> >> better > >> >> option. (while in this case it was fine - it's sort of a community > >> >> habit/standards thing - we don't want to create the idea that lack of > >> >> feedback is consent/approval in the review process) > >> >> > >> >> On Sun, Jun 12, 2016 at 7:01 PM, Mike Spertus via cfe-commits > >> >> <cfe-commits@lists.llvm.org> wrote: > >> >>> > >> >>> mspertus closed this revision. > >> >>> mspertus added a comment. > >> >>> > >> >>> revision 272525 > >> >>> > >> >>> > >> >>> http://reviews.llvm.org/D21256 > >> >>> > >> >>> > >> >>> > >> >>> _______________________________________________ > >> >>> cfe-commits mailing list > >> >>> cfe-commits@lists.llvm.org > >> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > >> >> > >> >> > >> > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
