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
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 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
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
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
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 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
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, 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
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
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
Re: [PATCH] D21256: Improved Visual Studio 2015 visualization of SmallVectorImpl
mspertus accepted this revision. mspertus added a reviewer: mspertus. mspertus added a comment. This revision is now accepted and ready to land. No adverse (or otherwise) comments received, so committing as 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
[PATCH] D21256: Improved Visual Studio 2015 visualization of SmallVectorImpl
mspertus created this revision.
mspertus added reviewers: aaron.ballman, zturner, aemerson.
mspertus added a subscriber: cfe-commits.
When visualizing small vectors in VS2015, show the first few elements in the
DisplayString instead of the size. For example, a `SmallVector` of
`DeclAccessPair` will visualize like
{public typename ...Ts, public typename U}
The visualization in VS2013 remains the same because we continue to include the
old visualizer with a lower-than-default priority of `MediumLow`, and the same
`SmallVector` would continue to be visualized as
{size = 2}
I decided to submit this one for review before commit because as `SmallVector`
is used pretty much everywhere in LLVM and Clang, even though I think this is a
huge improvement in practice but wanted to give others a chance to pipe in if
they disagree. If no one pipes in, I plan to go ahead and commit in 48 hours
http://reviews.llvm.org/D21256
Files:
llvm.natvis
Index: llvm.natvis
===
--- llvm.natvis
+++ llvm.natvis
@@ -8,8 +8,8 @@
For later versions of Visual Studio, no setup is required.
-->
http://schemas.microsoft.com/vstudio/debugger/natvis/2010";>
-
-
+
+
empty
{{
size={($T1*)EndX - ($T1*)BeginX} }}
@@ -21,6 +21,29 @@
+
+
+
+{(($T1*)BeginX)[0]}{*this,view(elt1)}
+
+,
{(($T1*)BeginX)[1]}{*this,view(elt2)}
+
+,
{(($T1*)BeginX)[2]}{*this,view(elt3)}
+
+,
{(($T1*)BeginX)[2]}{*this,view(elt4)}
+
+, /* {(($T1*)EndX - ($T1*)BeginX) - 4}
more*/
+empty
+{{{*this,view(elt0)}}}
+
+ ($T1*)EndX - ($T1*)BeginX
+ ($T1*)CapacityX - ($T1*)BeginX
+
+($T1*)EndX - ($T1*)BeginX
+($T1*)BeginX
+
+
+
empty
{{ size={Length} }}
Index: llvm.natvis
===
--- llvm.natvis
+++ llvm.natvis
@@ -8,8 +8,8 @@
For later versions of Visual Studio, no setup is required.
-->
http://schemas.microsoft.com/vstudio/debugger/natvis/2010";>
-
-
+
+
empty
{{ size={($T1*)EndX - ($T1*)BeginX} }}
@@ -21,6 +21,29 @@
+
+
+
+{(($T1*)BeginX)[0]}{*this,view(elt1)}
+
+, {(($T1*)BeginX)[1]}{*this,view(elt2)}
+
+, {(($T1*)BeginX)[2]}{*this,view(elt3)}
+
+, {(($T1*)BeginX)[2]}{*this,view(elt4)}
+
+, /* {(($T1*)EndX - ($T1*)BeginX) - 4} more*/
+empty
+{{{*this,view(elt0)}}}
+
+ ($T1*)EndX - ($T1*)BeginX
+ ($T1*)CapacityX - ($T1*)BeginX
+
+($T1*)EndX - ($T1*)BeginX
+($T1*)BeginX
+
+
+
empty
{{ size={Length} }}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
