Re: [PATCH] D21256: Improved Visual Studio 2015 visualization of SmallVectorImpl
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. 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? Thanks, Mike On Mon, Jun 13, 2016 at 10:16 AM, David Blaikie 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
Re: [PATCH] D21256: Improved Visual Studio 2015 visualization of SmallVectorImpl
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 wrote: > On Mon, Jun 13, 2016 at 12:27 PM, Zachary Turner > 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 > > wrote: > >> > >> On Mon, Jun 13, 2016 at 11:55 AM, Michael Spertus > >> 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 > >> > 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 > >> >> 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
Re: [PATCH] D21256: Improved Visual Studio 2015 visualization of SmallVectorImpl
Hi David, IIUC Eric Feiveson drives Visual Studio visualizers. I'll email him (and will also demo to STL in Oulu). Best, Mike On Monday, June 13, 2016, David Blaikie wrote: > > > On Mon, Jun 13, 2016 at 8:55 AM, Michael Spertus > 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. >> > > Oh, I don't use MSVC at all, so it's totally up to you, I'd just be > curious if the visualizers for SmallVector were different for those of > std::vector. Not that the authors of the inbuilt visualizers in MSVC have a > monopoly on correct/good design here. > > Might be worth roping STL (Stephan) into the thread to discuss MSVC > visualizers of the STL - and/or filing a bug, if we think there are better > ways to visualize containers than those provided by MSVC. > > >> >> 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? >> >> Thanks, >> >> Mike >> >> On Mon, Jun 13, 2016 at 10:16 AM, David Blaikie > > 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
Re: [PATCH] D21256: Improved Visual Studio 2015 visualization of SmallVectorImpl
On Mon, Jun 13, 2016 at 11:27 AM, Zachary Turner 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 > wrote: > >> On Mon, Jun 13, 2016 at 11:55 AM, Michael Spertus >> 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 >> 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 >> >> 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
