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