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.<br><br>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<br><div class="gmail_quote"><div dir="ltr">On Mon, Jun 13, 2016 at 9:10 AM Aaron Ballman <<a href="mailto:aaron.ballman@gmail.com">aaron.ballman@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Mon, Jun 13, 2016 at 11:55 AM, Michael Spertus <<a href="mailto:mike@spertus.com" target="_blank">mike@spertus.com</a>> wrote:<br>
> Hi David,<br>
> While I understand the initial reasoning. I have found that this is like 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 each<br>
> time you take a step through the code is really helpful. If you want me to<br>
> back it out and rereview we can, but I'd encourage you to try it out 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. 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, 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 as a<br>
> heads up. Should I change that to be if I don't feel comfortable 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" target="_blank">dblaikie@gmail.com</a>> 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. It<br>
>> sounds like the feedback you were looking for probably would've been fine a<br>
>> post-commit review feedback just as easily & perhaps might've been a 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" target="_blank">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" target="_blank">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>
</blockquote></div>