> On May 10, 2019, 3:31 a.m., Chun-Hung Hsiao wrote:
> > src/tests/utils/task_scheduler.cpp
> > Lines 289 (patched)
> > <https://reviews.apache.org/r/70579/diff/1/?file=2142143#file2142143line289>
> >
> > ```
> > offers_.erase(std::remove_if(...), offers_.end());
> > ```
> > Although the effect is the same since there will be only one element
> > removed, pairing `std::remove_if` and `end` will be more idomatic.
>
> Benjamin Mahler wrote:
> Hm.. why is this an "issue" and why is that more idiomatic..? Erase has
> two versions, one take a position and one takes a range.
>
> Looking at this code again, it's rather unreadable, and I probably will
> just write the version with a loop to find the one to erase.
The reason it's more idiomatic is because `it = std::remove_if(...)` returns an
iterator that points to the new end of valid items, which means that, `[it,
offers_.end())` should be removed. Since internally `std::remove_if` moves
items past `it` to their "correct" positions before `it`, erasing only `it` but
not things after it seems very strange at a first glance.
Calling `erase` with one iterator works here because we're relying on the fact
that `std::remove_if` will only remove one item in this particular case, which
is not trivial to me (before looking into what `offers_` contains and when the
lambda returns true.
If you prefer to use `erase` with one position, how about the following:
```
offers_.erase(std::find_if(
offers_.begin(),
offers_.end(),
[&](const v1::Offer& offer) {
...
}));
```
Also, I just noticed that both the snippet you have and the one I wrote above
relies on the invariant where `event.rescind().offer_id()` exists in `offers_`.
`vector::erase(pos)` requires that the iterator passed in should be both valid
and deferenceable. If there are more than one agents, then this invariant will
be violated.
- Chun-Hung
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70579/#review215174
-----------------------------------------------------------
On May 1, 2019, 9:17 p.m., Benjamin Mahler wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70579/
> -----------------------------------------------------------
>
> (Updated May 1, 2019, 9:17 p.m.)
>
>
> Review request for mesos, Chun-Hung Hsiao and Greg Mann.
>
>
> Bugs: MESOS-8511
> https://issues.apache.org/jira/browse/MESOS-8511
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The caller can submit tasks to the scheduler and get back statuses.
> This reduces the boilerplate of using the low level scheduler
> library.
>
>
> Diffs
> -----
>
> include/mesos/v1/scheduler.hpp 2f72129ba48943c891d7020bfd2cad3066355026
> src/Makefile.am 5f97523fbe2d80733fbdcc7706f2761f5a071f9f
> src/scheduler/scheduler.cpp 674483aa80bc74b654343c97892a96f49d5c7ed4
> src/tests/CMakeLists.txt e6b1d8a097246f0921467d7f778034d6754fcff6
> src/tests/utils/task_scheduler.hpp PRE-CREATION
> src/tests/utils/task_scheduler.cpp PRE-CREATION
>
>
> Diff: https://reviews.apache.org/r/70579/diff/1/
>
>
> Testing
> -------
>
> Tested it via the subsequent patch.
>
>
> Thanks,
>
> Benjamin Mahler
>
>
