Duncan P. N. Exon Smith
2014-Jul-15 18:28 UTC
[LLVMdev] [cfe-dev] Bug in MapVector::erase ?
> On 2014-Jul-15, at 11:07, Reid Kleckner <rnk at google.com> wrote: > > Can we explicitly delete the erase method or do something else to document the fact that it is unsupported? It was added incidentally in r211350, even though it was added and removed by Doug back in r175538 / r175449. >I'm happy with it deleted or fixed (see WIP patch that fixes it w/o tests). For now, I'll fix it, and then David (or someone else) can migrate the code to `remove_if()`. FWIW, when it's fixed, it doesn't have to be "unsupported" -- it's just *slow*. -------------- next part -------------- A non-text attachment was scrubbed... Name: mapvector.patch Type: application/octet-stream Size: 1999 bytes Desc: not available URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140715/00ee4949/attachment.obj>
Duncan P. N. Exon Smith
2014-Jul-15 20:42 UTC
[LLVMdev] [cfe-dev] Bug in MapVector::erase ?
> On 2014-Jul-15, at 11:28, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote: > > >> On 2014-Jul-15, at 11:07, Reid Kleckner <rnk at google.com> wrote: >> >> Can we explicitly delete the erase method or do something else to document the fact that it is unsupported? It was added incidentally in r211350, even though it was added and removed by Doug back in r175538 / r175449. >> > > I'm happy with it deleted or fixed (see WIP patch that fixes it w/o > tests). For now, I'll fix it, and then David (or someone else) can > migrate the code to `remove_if()`. > > FWIW, when it's fixed, it doesn't have to be "unsupported" -- it's > just *slow*.Committed a whitespace change in r213082, fixed `erase()` in r213084, and added `remove_if()` in r213090. I'll leave it up to David how to move forward once the MCDwarf stuff migrates to `remove_if()`. If re-deleting `erase()` is the plan, someone should update the comments and the docs right away to say so (I changed the text to document its complexity, rather than lack of support).
On Tue, Jul 15, 2014 at 1:42 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:> >> On 2014-Jul-15, at 11:28, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote: >> >> >>> On 2014-Jul-15, at 11:07, Reid Kleckner <rnk at google.com> wrote: >>> >>> Can we explicitly delete the erase method or do something else to document the fact that it is unsupported? It was added incidentally in r211350, even though it was added and removed by Doug back in r175538 / r175449. >>> >> >> I'm happy with it deleted or fixed (see WIP patch that fixes it w/o >> tests). For now, I'll fix it, and then David (or someone else) can >> migrate the code to `remove_if()`. >> >> FWIW, when it's fixed, it doesn't have to be "unsupported" -- it's >> just *slow*. > > Committed a whitespace change in r213082, fixed `erase()` in r213084, and added `remove_if()` in r213090. > > I'll leave it up to David how to move forward once the MCDwarf stuff > migrates to `remove_if()`. > > If re-deleting `erase()` is the plan, someone should update the comments > and the docs right away to say so (I changed the text to document its > complexity, rather than lack of support).Doubt I'll get to making any changes immediately (including switching over to remove_if - perhaps Oscar can send a CR for that) but will keep it all in mind :) Thanks again! - Dave
It's worth keeping erase() with the complexity comment, same as we have linear-time vector find(). All ADT have tradeoffs. We could have constant time erase with a MapList instead of MapVector but the random access iterator would be linear-time. Yaron 2014-07-15 23:42 GMT+03:00 Duncan P. N. Exon Smith <dexonsmith at apple.com>:> > > On 2014-Jul-15, at 11:28, Duncan P. N. Exon Smith <dexonsmith at apple.com> > wrote: > > > > > >> On 2014-Jul-15, at 11:07, Reid Kleckner <rnk at google.com> wrote: > >> > >> Can we explicitly delete the erase method or do something else to > document the fact that it is unsupported? It was added incidentally in > r211350, even though it was added and removed by Doug back in r175538 / > r175449. > >> > > > > I'm happy with it deleted or fixed (see WIP patch that fixes it w/o > > tests). For now, I'll fix it, and then David (or someone else) can > > migrate the code to `remove_if()`. > > > > FWIW, when it's fixed, it doesn't have to be "unsupported" -- it's > > just *slow*. > > Committed a whitespace change in r213082, fixed `erase()` in r213084, and > added `remove_if()` in r213090. > > I'll leave it up to David how to move forward once the MCDwarf stuff > migrates to `remove_if()`. > > If re-deleting `erase()` is the plan, someone should update the comments > and the docs right away to say so (I changed the text to document its > complexity, rather than lack of support). > _______________________________________________ > cfe-dev mailing list > cfe-dev at cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140715/83b4952a/attachment.html>