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