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

[Michael Spertus via cfe-commits]

On Mon, Jun 13, 2016 at 11:27 AM, 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


I agree with this (which is why I did it the way I did in the first
place!).  While I'm not planning on doing a "heads up" diff going forward
unless there is a change in community standards, do we want to consider
such a change in community standards? How would that happen?

Thanks,
Mike


> 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/20160614/ec0b9fa1/attachment-0001.html>