Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace std::bind with lambdas #4017

Merged
merged 2 commits into from
Mar 18, 2022
Merged

Conversation

ranisalt
Copy link
Member

@ranisalt ranisalt commented Mar 15, 2022

Pull Request Prelude

Changes Proposed

std::bind is bad for several reasons:

  • The code is not really clear, the usage of placeholders makes it difficult to follow
  • The complexity of the implementation of std::bind often results in sub-optimal code
  • The possibilities offered by bind are limited, for instance, it’s not possible to bind a function with variable arguments allows to silently discard some arguments, which is often not what was expected - see in connection.cpp how in some calls it simply ignored a parameter, with lambdas we have to write the proper callback param types
  • Fixed arguments are evaluated when bind is called, not when the bound function is called, which might be surprising in some cases - it is also considerably slower to compile
  • bind requires to take the address of a function, which can be difficult (if the function is overloaded) or not recommended (for standard library functions)

(source)

See also detailed explanation by Jason Turner.

Additionally, move non-trivial types into lambdas to avoid expensive copies for temporary values, and used lambdas where we tried to work around std::bind's lack of flexibility.

MillhioreBT
MillhioreBT previously approved these changes Mar 16, 2022
EPuncker
EPuncker previously approved these changes Mar 16, 2022
@MillhioreBT
Copy link
Contributor

MillhioreBT commented Mar 16, 2022

problem solved with Fix potential bad weak_ptr usage commit

Sorry to bother but I've been testing the changes a bit more and I ran into a problem, I don't know if it's due to my custom sources or if this could also be happening in TFS.
image

Everything compiles successfully, you can even enter with any player, however when disconnecting the player this error can happen, when I place a breakpoint here it increases the probability that the error will not occur
image

Note: In fact, if I do it, I go through the interruption points very slowly, clicking the Continue button every 5 seconds or more, there is no problem.
But when I hit the continue button quickly the problem occurs
This is what happens when I loop through the breakpoints quickly:
image

Basically the call stack is as follows:

ProtocolGame::logout
Protocol::disconnect
Connection::close
and then the call to
ProtocolGame::release() in lambda g_dispatcher.addTask(createTask([=]() { protocol->release(); }));
finally the error seems to occur on this call
OutputMessagePool::getInstance().removeProtocolFromAutosend(shared_from_this());

@ranisalt ranisalt dismissed stale reviews from MillhioreBT and EPuncker via 94d95ca March 16, 2022 11:06
Copy link
Member

@DSpeichert DSpeichert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I feel like we should have a style where the lambda body is always on its own lines, some of those "short" one-liners are actually long eye sores, is there a formatting standard we can adopt, since lambdas are not common in the codebase yet?

src/connection.cpp Show resolved Hide resolved
@ranisalt
Copy link
Member Author

@DSpeichert that's definitely what I'm doing next. I spoke to @EPuncker about a standard code style + static analysis + unit testing, however that would be a separate PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants