[Lldb-commits] [PATCH] D74010: Fix BroadcasterManager::RemoveListener to really remove the listener

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

Wow, cool bug.

It's too bad the original code re-used an iterator variable instead of make a new name (which would have helped the compiler find the problem).  Note that the one they used is shadowed just a couple lines later.

It's too bad the original code feels it's necessary to create iter and end_iter up front.  I know raw loops require this trick to avoid re-computing the end iterator on each iteration through the loop, but that shouldn't be necessary on algorithms like `find_if`.

It's too bad that erase with an end iterator isn't just a safe no-op, so that zillions of callers aren't required to check find's return value.  Without the visual noise, it would be easier to write exactly what you want.

It's too bad the compiler cannot recognize that `find_if` has no side effects and thus ignoring its return value makes the statement a no-op.

It's too bad the `std::erase(std::remove_if(...))` idiom is so cumbersome.  I realize that would likely be overkill here, since you apparently want to erase just the first one that matches the predicate.  Nonetheless, it would be harder to make this kind of bug.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74010/new/

https://reviews.llvm.org/D74010