[PATCH] D21256: Improved Visual Studio 2015 visualization of SmallVectorImpl

[Michael Spertus via cfe-commits]

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.ballman at gmail.com>
wrote:

> On Mon, Jun 13, 2016 at 12:27 PM, Zachary Turner <zturner at 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.ballman at gmail.com>
> > wrote:
> >>
> >> On Mon, Jun 13, 2016 at 11:55 AM, Michael Spertus <mike at 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 <dblaikie at 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 at 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 at lists.llvm.org
> >> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
> >> >>
> >> >>
> >> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160613/96f8bad5/attachment.html>