std::erase_if not working properly

[graceful mirage]

Here's the code

module;

#include <iostream>
#include <chrono>
#include <vector>

export module Main;

template<class T>
class Timed final {
private:
    T _element;
    std::chrono::milliseconds _time;

public:
    Timed(T&& element, long t) : _element(std::forward<T>(element)), _time(t) { }

    ~Timed() = default;

    // Copy Constructor
    Timed(const Timed& other) : _element(other._element), _time(other._time) { }

    // Copy Assignment Operator
    Timed& operator=(const Timed& other)
    {
        if (this != &other)
        {
            _element = other._element;
            _time = other._time;
        }
        return *this;
    }

    // Move Constructor
    Timed(Timed&& other) noexcept : _element(std::move(other._element)), _time(other._time) { }

    // Move Assignment Operator
    Timed& operator=(Timed&& other) noexcept
    {
        if (this != &other)
        {
            _element = std::move(other._element);
            _time = other._time;
        }
        return *this;
    }

    [[nodiscard]] const T& GetElement() const noexcept
    {
        return _element;
    }

    [[nodiscard]] const std::chrono::milliseconds& GetTime() const noexcept
    {
        return _time;
    }
};

template<typename T>
bool check_erase(std::vector<Timed<T>>& vector, const std::chrono::time_point<std::chrono::steady_clock>& startTime)
{
    const auto currentTime{ std::chrono::steady_clock::now() };
    const auto res{ std::erase_if(vector, [&](Timed<T>& t) -> bool
    {
        return std::chrono::duration_cast<std::chrono::milliseconds>(currentTime - startTime) > t.GetTime();
    }) > 0 };

    if (res)
    {
        std::cout << "deleting\n";
    }

    return res;
}

export int main()
{
    std::vector<Timed<int>> elements{
            { 1, 3000 },
            { 2, 1000 },
    };

    const auto startTime{ std::chrono::steady_clock::now() };
    while (!elements.empty())
    {
        check_erase(elements, startTime);
    }

    return 0;
}

[tall coral]

When your question is answered use !solved to mark the question as resolved.

Remember to ask specific questions, provide necessary details, and reduce your question to its simplest form. For tips on how to ask a good question use !howto ask.

[graceful mirage]

My objective is to have a container of elements and remove specific elements after a period of time, however I have an issue when calling check_erase

[mossy junco]

first thing I notice: main must not be attached to a module

[graceful mirage]

The following condition

const auto res{ std::erase_if(vector, [&](Timed<T>& t) -> bool
{
    return std::chrono::duration_cast<std::chrono::milliseconds>(currentTime - startTime) > t.GetTime();
}) > 0 };

Only works when all the elements inside the vector satisfy that condition, thus all elements get removed immediately

For example, consider this container

std::vector<Timed<int>> elements{
        { 1, 3000 },
        { 2, 1000 },
        { 3, 5000 },
};

I have 3 elements with a different time to remove each.
Normally it should behave like that

1. Delete 2nd element after 1s
2. Delete 1st element after 3s
3. Delete 3rd element after 5s

However it deletes all the elements when 5s are elapsed

[graceful mirage]

What do you mean?

[mossy junco]

you export int main() from your module

[graceful mirage]

yeah, but if I don't export it I get the following error
MSVCRTD.lib(exe_main.obj) : error LNK2019: unresolved external symbol main referenced in function "int __cdecl invoke_main(void)" (?invoke_main@@YAHXZ)

[violet delta]

What if you use std::endl instead of "\n"? For your "deleting" print. That'll forcefully flush the buffer in case that is why you are getting all the prints at the same time.

Or are you observing in a debugger that at 5s the vector goes from 3 elements to 0?

[mossy junco]

I think if you declare it extern "C++", it becomes attached to the global module instead of the module it appears in

[graceful mirage]

I can't believe that
It was a flushing issue haha

[mossy junco]

but having it in an interface unit is slightly weird

[violet delta]

I've been there. 😅

[graceful mirage]

how come?

while we're at it, anything I can improve?

[mossy junco]

well, an interface unit is for exporting stuff so that others can import it

but main is something no one is allowed to use

so why would anyone import it

[graceful mirage]

I wanted to take advantages of modules
but it doesn't make a lot of sense in main right

@mossy junco in this class should I remove std::move in move constructor and move assignment operator?

template<class T>
class Timed final {
private:
    T _element;
    std::chrono::milliseconds _time;

public:
    Timed(T&& element, long t) noexcept : _element{ std::forward<T>(element) }, _time{ t } { }

    ~Timed() noexcept = default;

    // Copy Constructor
    Timed(const Timed& other) noexcept : _element{ other._element }, _time{ other._time } { }

    // Copy Assignment Operator
    Timed& operator=(const Timed& other) noexcept
    {
        if (this != &other)
        {
            _element = other._element;
            _time = other._time;
        }
        return *this;
    }

    // Move Constructor
    Timed(Timed&& other) noexcept : _element{ std::move(other._element) }, _time{ other._time } { }

    // Move Assignment Operator
    Timed& operator=(Timed&& other) noexcept
    {
        if (this != &other)
        {
            _element = std::move(other._element);
            _time = other._time;
        }
        return *this;
    }

    [[nodiscard]] const T& GetElement() const noexcept
    {
        return _element;
    }

    [[nodiscard]] const std::chrono::milliseconds& GetTime() const noexcept
    {
        return _time;
    }
};

since it's being passed by rvalue reference

or is it a good practice to keep std::move

[mossy junco]

it's necessary if you want to move _element

[graceful mirage]

ah I see it

because Timed is being passed as a rvalue refence, but _element is not

[mossy junco]

also note that your constructor here: Timed(T&& element, long t)
is not a forwarding constructor, because T is not deduced from the callsite, it's exactly whatever the T argument of the class template is + &&

i.e. Timed<int> expects an int&& argument

[graceful mirage]

so it will not take advantage of move semantics whether I pass an lvalue or rvalue?

[mossy junco]

it will not allow passing in anything but an rvalue ref

unless you have something like Timed<int&>

I'm just saying the std::forward there is a bit misleading even if it doesn't do anything wrong

[graceful mirage]

so what's a correct implementation if I may ask

[mossy junco]

you could write const T&, T&& overloads,
you could write one T overload that always moves,
you could write a perfect-forwarding constructor template like

template<typename U>
requires std::is_nothrow_convertible<U&&, T>
Timed(U&& u, long t) noexcept : _element{std::forward<U>(u)} ..... {}

you could also = default at least the move/copy constructors,
the assignments too if you don't care about the self-assignment too strongly

is there any particular reason to take the time as a plain long instead of milliseconds or any duration?

(also, nothing seems to modify this time, what is it for? do you need a fancy class with getters if there are no invariants to maintain?)

[graceful mirage]

not really

[graceful mirage]

I created those getters because the copy assignment would give me an error if _time was a constant

[mossy junco]

yeah const members are problematic like that :v

[graceful mirage]

anyways I'll use the const T&, T&&, I still don't really understand the perfect-forwading constructor template you sent me

like why does with requires std::is_nothrow_convertible<U&&, T> work?

[mossy junco]

that's just asserting that what you want to do (constructing a T from whatever U&& is) is possible and noexcept

since you made the constructor noexcept, we don't want to lie 😄

(otherwise, this may std::terminate if it does throw)

since U can be anything, this restricts it to only things that can actually be converted to T

[dull laurel]

xD
Yeah using std::endl instead of a '\n' is an issue when using loops especially