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

Wider use of lambdas #3948

Merged
merged 4 commits into from Sep 1, 2017

Conversation

Projects
None yet
3 participants
@peterbell10
Copy link
Member

peterbell10 commented Aug 22, 2017

  • Replace cItemCallback with cFunctionRef std::function for lambda compatibility
  • Replace some use of "ad hoc functor classes" with lambdas.

I also took the opportunity to change the signature of the DoWith and ForEach callbacks to take references.

@peterbell10 peterbell10 force-pushed the peterbell10:Lambdas branch from 7d4a7bd to 7d0f4f4 Aug 22, 2017

/** A light-weight, type-erased reference to a function object.
Non-owning, be mindful of lifetimes. */
template <class Ret, class... Args>
class cFunctionRef<Ret(Args...)>

This comment has been minimized.

@madmaxoft

madmaxoft Aug 22, 2017

Member

Why exactly do we need a new object for this? Shouldn't using std::function suffice? What is the use-case that forces us to use this?

This comment has been minimized.

@peterbell10

peterbell10 Aug 22, 2017

Member

While a std::function would work, it is comparatively heavy-weight. It has to copy the object which might require heap allocations (depending on the implementations small functor optimisation limit). This on the other hand has (or at least should have) no overhead compared to the inheritance method.

Not as much of a motivation but this will also support non-copyable functors.

This comment has been minimized.

@madmaxoft

madmaxoft Aug 25, 2017

Member

Can you elaborate with an example? I still don't get it :(

This comment has been minimized.

@peterbell10

peterbell10 Aug 25, 2017

Member

Sorry but which part don't you get? Do you mean why std::function might allocate memory?

This comment has been minimized.

@madmaxoft

madmaxoft Aug 25, 2017

Member

I'd like an example for which you felt std::function was not good enough (or unusable) and which the new framework fixes.

This comment has been minimized.

@peterbell10

peterbell10 Aug 25, 2017

Member

It's mainly for performance reasons. Similar to std::string vs std::string_view.
I can leave it out and use std::function if you'd prefer.

This comment has been minimized.

@madmaxoft

madmaxoft Aug 25, 2017

Member

It seems like a premature optimization to me. Let's go with std::function for now and if it proves to be a bottleneck in the future, we can revisit this framework.

This comment has been minimized.

@peterbell10

peterbell10 Aug 25, 2017

Member

Categorising 50 lines of code as a "framework" seems a bit extreme.

This comment has been minimized.

@madmaxoft

madmaxoft Aug 25, 2017

Member

Yeah, but it's dense code, quite difficult to understand what it actually does :) Which is exactly the definition of framework (or library - ever looked into STL? :)

This comment has been minimized.

@peterbell10

peterbell10 Aug 25, 2017

Member

I could add more documentation in cFunctionRef if that helps. The principle is very simple and it isn't doing much really.

It's not much different from the c-style stuff used in the network code.

@peterbell10 peterbell10 force-pushed the peterbell10:Lambdas branch 5 times, most recently from 5228b8f to a0880ed Aug 22, 2017

@peterbell10

This comment has been minimized.

Copy link
Member

peterbell10 commented Aug 24, 2017

Would it be easier if I split this into multiple PRs? One adding support for lambdas and separate ones for the use of lambdas.

@madmaxoft

This comment has been minimized.

Copy link
Member

madmaxoft commented Aug 25, 2017

I don't think splitting is necessary, it'll just take me some time to absorb the new framework's inner workings; after that it's gonna be fast (I hope).

@peterbell10

This comment has been minimized.

Copy link
Member

peterbell10 commented Aug 25, 2017

I'm just slightly worried about how likely conflicts are as this is such a fundamental functionality.

@peterbell10 peterbell10 force-pushed the peterbell10:Lambdas branch from a0880ed to 06a2c09 Aug 25, 2017

@madmaxoft

This comment has been minimized.

Copy link
Member

madmaxoft commented Aug 25, 2017

You won't avoid conflicts by splitting the new-file away from this PR :)

peterbell10 added some commits Jul 27, 2017

Wider use of lambdas
  * Replace cItemCallback with cFunctionRef for lambda compatibility
  * Replace some use of "ad hoc functor classes" with lambdas.

@peterbell10 peterbell10 force-pushed the peterbell10:Lambdas branch from 06a2c09 to 7f2f0e4 Aug 25, 2017

@peterbell10 peterbell10 force-pushed the peterbell10:Lambdas branch from 77d7a57 to 1ce9926 Aug 26, 2017

@madmaxoft madmaxoft merged commit 496c337 into cuberite:master Sep 1, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Seadragon91

This comment has been minimized.

Copy link
Contributor

Seadragon91 commented Sep 1, 2017

We really need tests... . None of the player commands from Core works: e.g. /gm or /give. They uses the API function cRoot:Get():FindAndDoWithPlayer

Edit: The function cWorld:FindAndDoWithPlayer works

@peterbell10

This comment has been minimized.

Copy link
Member

peterbell10 commented Sep 2, 2017

I think I just found the need for cFunctionRef...
cRoot::FindAndDoWithPlayer uses a mutable callback which the std::function takes a copy of so the state of the original callback is never updated. With cFunctionRef the lack of copy would keep this working.

SafwatHalaby pushed a commit that referenced this pull request Sep 2, 2017

LogicParrot LogicParrot

bearbin added a commit that referenced this pull request Sep 2, 2017

@peterbell10 peterbell10 deleted the peterbell10:Lambdas branch Sep 2, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment