[Lldb-commits] [PATCH] D74010: Fix BroadcasterManager::RemoveListener to really remove the listener
Adrian McCarthy via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Wed Feb 5 09:09:24 PST 2020
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
More information about the lldb-commits
mailing list