fix(init): register AV connects and call after AV is ready

[anthonybilinski]

Fixes #4406

---

This change is 

[Talkless]

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

src/core/core.cpp, line 248 at r1 (raw file):

        emit failedToStart();
    }
    for (auto callback : toCallWhenAvReady) {

const auto& callback I believe, not sure why would it need a copy.

---

src/core/core.cpp, line 249 at r1 (raw file):

    }
    for (auto callback : toCallWhenAvReady) {
    	callback(av);

If this >> means tab here in Reviewable? Then it should be replaced to spaces.

---

src/core/core.cpp, line 1395 at r1 (raw file):

}

void Core::callWhenAvReady(std::function<void(CoreAV* av)> toCall)

Should be const &, std::function likely to have dynamic allocations, would be nice to avoid additional copies here. It will be copied into container later once.

Or, you assume that here gonna be two moves? (into callWehnAvReady() and into empalce_back())?

If yes, I am kinda sure that you need .emplace_back(std::move(toCall)) because toCall is a variable with a name (lvalue), so emplace_back() will simply use copy constructor.

---

src/core/core.cpp, line 1397 at r1 (raw file):

void Core::callWhenAvReady(std::function<void(CoreAV* av)> toCall)
{
	toCallWhenAvReady.emplace_back(toCall);

Spaces, no tabs.

---

Comments from Reviewable

[anthonybilinski]

Review status: 0 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

src/core/core.cpp, line 248 at r1 (raw file):

Previously, Talkless (Vincas Dargis) wrote…

const auto& callback I believe, not sure why would it need a copy.

Done.

---

src/core/core.cpp, line 249 at r1 (raw file):

Previously, Talkless (Vincas Dargis) wrote…

If this >> means tab here in Reviewable? Then it should be replaced to spaces.

Done.

---

src/core/core.cpp, line 1395 at r1 (raw file):

Previously, Talkless (Vincas Dargis) wrote…

Should be const &, std::function likely to have dynamic allocations, would be nice to avoid additional copies here. It will be copied into container later once.

Or, you assume that here gonna be two moves? (into callWehnAvReady() and into empalce_back())?

If yes, I am kinda sure that you need .emplace_back(std::move(toCall)) because toCall is a variable with a name (lvalue), so emplace_back() will simply use copy constructor.

changed to const &, this removes the need for std::move, correct?

---

src/core/core.cpp, line 1397 at r1 (raw file):

Previously, Talkless (Vincas Dargis) wrote…

Spaces, no tabs.

Done.

---

Comments from Reviewable

[sudden6]

please someone else also take a look

---

Reviewed 1 of 3 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[noavarice]

Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

src/core/core.cpp, line 1395 at r1 (raw file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

changed to const &, this removes the need for std::move, correct?

AFAIC, std::move(toCall) must not work here anyway, because toCall is a constant object which cannot be moved

---

Comments from Reviewable

[Talkless]

Review status: all files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

---

src/core/core.cpp, line 1395 at r1 (raw file):

}

void Core::callWhenAvReady(std::function<void(CoreAV* av)> toCall)

You need to take rvalue reference: std::function<void(CoreAV* av)>&& toCall) and still std::move()-it into container. It's strange (welp, it's C++ :-) ), but parameter is rvalue reference for the callers, not for the function body itself.

See for sink function example: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#f18-for-consume-parameters-pass-by-x-and-stdmove-the-parameter

If it has a name, it's lvalue reference, and not gonna be moved from by default (unless it's in return statement AFAIK, local variable can be moved into function result).

---

Comments from Reviewable

[anthonybilinski]

Review status: 1 of 3 files reviewed at latest revision, 5 unresolved discussions, some commit checks pending.

---

src/core/core.cpp, line 1395 at r1 (raw file):

Previously, noavarice wrote…

AFAIC, std::move(toCall) must not work here anyway, because toCall is a constant object which cannot be moved

It compiles, FYI. Not sure why..

---

src/core/core.cpp, line 1395 at r1 (raw file):

Previously, Talkless (Vincas Dargis) wrote…

You need to take rvalue reference: std::function<void(CoreAV* av)>&& toCall) and still std::move()-it into container. It's strange (welp, it's C++ :-) ), but parameter is rvalue reference for the callers, not for the function body itself.

See for sink function example: https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#f18-for-consume-parameters-pass-by-x-and-stdmove-the-parameter

If it has a name, it's lvalue reference, and not gonna be moved from by default (unless it's in return statement AFAIK, local variable can be moved into function result).

Changed from const& to rvalue reference, with a std::move into container, as per cpp core guidelines recommendation linked. Thanks for the in-depth feedback :)

---

Comments from Reviewable

[noavarice]

Review status: 1 of 3 files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

---

src/core/core.cpp, line 1395 at r1 (raw file):

Previously, anthonybilinski (Anthony Bilinski) wrote…

It compiles, FYI. Not sure why..

It's not about compiling, this just will do the copy of toCall silently

---

Comments from Reviewable

[Talkless]

---

Reviewed 1 of 3 files at r1, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

Comments from Reviewable

[Diadlo]

---

Reviewed 1 of 3 files at r1, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[anthonybilinski]

rebased back to tip

[noavarice]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

src/core/core.h, line 230 at r4 (raw file):

    bool ready;
    const ICoreSettings* const s;
    std::vector<std::function<void(CoreAV* av)>> toCallWhenAvReady;

Why not std::list if we only need to push items back and iterate through them?

---

Comments from Reviewable

[noavarice]

Reviewed 3 of 3 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[Talkless]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

src/core/core.h, line 230 at r4 (raw file):

Previously, noavarice wrote…

Why not std::list if we only need to push items back and iterate through them?

And how std::list is better in this case? std::vector should be default container by all the C++ Gods recommendations :-) . std::list is double-linked list, meaning every node is dynamically allocated, with lot's of indirections through pointers when you "only" iterate.

One should use std::list when you do a lot of prepending, inserting into the middle, and need stable references when removing (AFAIK), and probably argued with a benchmark on top of it :-) .

---

Comments from Reviewable

[noavarice]

Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

src/core/core.h, line 230 at r4 (raw file):

Previously, Talkless (Vincas Dargis) wrote…

And how std::list is better in this case? std::vector should be default container by all the C++ Gods recommendations :-) . std::list is double-linked list, meaning every node is dynamically allocated, with lot's of indirections through pointers when you "only" iterate.

One should use std::list when you do a lot of prepending, inserting into the middle, and need stable references when removing (AFAIK), and probably argued with a benchmark on top of it :-) .

Got it)

---

Comments from Reviewable

[noavarice]