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

getTime refactoring #1188

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
10 participants
@selat
Copy link
Contributor

commented Mar 29, 2014

Some refactoring of getTime() functions.

@sapier

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2014

please rebase, doesn't apply to current master, breaks curl.

@sapier sapier added this to the 0.4.11 milestone Jun 29, 2014

@sapier sapier removed this from the 0.4.11 milestone Aug 16, 2014

@sapier

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2014

I'm removing the milestone and scheduling this for closing if not rebased till end of august

@selat

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2014

Rebased. And please recheck - I changed it a little bit.

@sapier sapier force-pushed the minetest:master branch 2 times, most recently to c24e075 Aug 19, 2014

@sapier

View changes

src/gettime.h Outdated
using porting::getTimeS;
using porting::getTimeMs;
using porting::getTimeUs;
using porting::getTimeNs;

This comment has been minimized.

Copy link
@sapier

sapier Aug 22, 2014

Contributor

Do you use those functions more then once? If not why not just prefix the namespace on usage?

This comment has been minimized.

Copy link
@selat

selat Aug 22, 2014

Author Contributor

Do you use those functions more then once? If not why not just prefix the namespace on usage?

This isn't only for gettime.h. I think that using porting namespace just to get time is strange.

This comment has been minimized.

Copy link
@sapier

sapier Aug 22, 2014

Contributor

On the other hand it'd be obvious that OS specific variants are used

This comment has been minimized.

Copy link
@selat

selat Aug 24, 2014

Author Contributor

If programmer includes gettime.h then he can expect that this file will provide getTime() functions, and no matter how these functions are implemented.

@sapier

View changes

src/main.cpp Outdated
@@ -1128,10 +1054,6 @@ int main(int argc, char *argv[])
if (run_dedicated_server)
{
DSTACK("Dedicated server branch");
// Create time getter if built with Irrlicht
#ifndef SERVER
g_timegetter = new SimpleTimeGetter();

This comment has been minimized.

Copy link
@sapier

sapier Aug 22, 2014

Contributor

What's this gonna do?

@sapier

View changes

src/main.cpp Outdated
@@ -1509,9 +1431,6 @@ int main(int argc, char *argv[])
*/
//driver->setMinHardwareBufferVertexCount(50);

// Create time getter
g_timegetter = new IrrlichtTimeGetter(device);

This comment has been minimized.

Copy link
@sapier

sapier Aug 22, 2014

Contributor

What's this gonna do?

This comment has been minimized.

Copy link
@selat

selat Aug 22, 2014

Author Contributor

What's this gonna do?

As I understand it g_timegetter implemented getTime() functions. But now it's not used.

@sapier sapier removed the rebase needed label Aug 22, 2014

@ShadowNinja ShadowNinja force-pushed the minetest:master branch 4 times, most recently to 56195dc Sep 20, 2014

@ShadowNinja ShadowNinja force-pushed the minetest:master branch from cba038e to b98e8d6 Oct 7, 2014

@kahrl

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2014

Don't remove the IrrlichtTimeGetter; it is more precise than the simple functions implemented in minetest.

@selat selat force-pushed the selat:gettime-refactoring branch 2 times, most recently Nov 15, 2014

@selat

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2014

Done (I've moved everything getTime()-related to gettime.cpp).

@Zeno- Zeno- force-pushed the minetest:master branch to a1db83e Nov 25, 2014

@kwolekr kwolekr force-pushed the minetest:master branch to 3f83ca2 Dec 25, 2014

@nerzhul

This comment has been minimized.

Copy link
Member

commented Feb 10, 2015

Hi @selat can you rebase ?

@selat selat force-pushed the selat:gettime-refactoring branch Feb 10, 2015

@selat

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2015

Rebased.

@nerzhul

This comment has been minimized.

Copy link
Member

commented Feb 10, 2015

I think using a static class like TimeMgr:: would be great, this permit to export it into a separate library.
@Zeno- what do you think about it ?

@kwolekr

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2015

I would like this merged, but it needs a rebase, some minor cleanup, and testing.

@est31 est31 force-pushed the minetest:master branch to a890c66 Aug 2, 2015

@nerzhul nerzhul added this to the 0.4.15 milestone May 14, 2016

@nerzhul

This comment has been minimized.

Copy link
Member

commented May 14, 2016

as me and @kwolekr wants to see this merged, tag it for next release

@paramat paramat removed this from the 0.4.15 milestone May 17, 2016

@SmallJoker

This comment has been minimized.

Copy link
Member

commented May 22, 2016

Looks good to me. Please rebase this pull, @selat

@selat

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2016

Can I remove IrrlichtTimeGetter and use functions from porting.h instead?

@SmallJoker

This comment has been minimized.

Copy link
Member

commented May 22, 2016

TimeGetter is only used in clientlaucher.c, so replacing it with the porting functions is okay.

@selat selat force-pushed the selat:gettime-refactoring branch 2 times, most recently May 22, 2016

@selat

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2016

Rebased.

@selat selat force-pushed the selat:gettime-refactoring branch 3 times, most recently to 5d674bf May 22, 2016

@selat

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2016

I think failing test isn't related to the pull request as there are no significant changes in threading-related files.

@paramat paramat removed the Rebase needed label May 22, 2016

@ShadowNinja

This comment has been minimized.

Copy link
Member

commented May 23, 2016

I actually re-implemented this change here for something else: 3625a31

Looks good, but could you merge in the u32->u64 change from my commit? It goes with this commit, and that way I don't have to rebase my commit.

Clean up getTime helpers
This increases size of the getTime return values to 64 bits.
It also removes the TimeGetter classes since the getTime functions
are now very precise.
@selat

This comment has been minimized.

Copy link
Contributor Author

commented May 23, 2016

I cherry picked your commit, but for some reason github doesn't see it. You can see this commit by cloning my repository and viewing log of gettime-refactoring branch.

@paramat

This comment has been minimized.

Copy link
Member

commented Oct 27, 2016

Bump.

@sfan5 sfan5 force-pushed the minetest:master branch to 09f1a0c Dec 21, 2016

@SmallJoker

This comment has been minimized.

Copy link
Member

commented Mar 24, 2017

Please reopen this pull if rebased. (Closed due inactivity)

@SmallJoker SmallJoker closed this Mar 24, 2017

@Ekdohibs Ekdohibs referenced this pull request Apr 15, 2017

Closed

getTime refactoring #5597

@Ekdohibs

This comment has been minimized.

Copy link
Member

commented Apr 15, 2017

Replaced by #5597.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.