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

compatibility with erlang 18 #10

Merged
merged 1 commit into from
May 15, 2016
Merged

compatibility with erlang 18 #10

merged 1 commit into from
May 15, 2016

Conversation

benoitc
Copy link
Contributor

@benoitc benoitc commented May 13, 2016

add compatibility with erlang 18 and sup. use erlang:timestamp() instead of erlang:now()



compat_now() ->
try erlang:timestamp()
Copy link
Owner

Choose a reason for hiding this comment

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

If called on an older erlang version, this will result in a call to code_server. Better to check using erlang:is_builtin(erlang, timestamp, 0). Example, using R16B03:

5> try erlang:timestamp() catch error:undef -> undefined end.
(<0.41.0>) call code_server:call(code_server,{ensure_loaded,erlang})
(<0.41.0>) returned from code_server:call/2 -> {module,erlang}
undefined
6> erlang:is_builtin(erlang,timestamp,0).
false
7> erlang:is_builtin(erlang,now,0).      
true

@benoitc
Copy link
Contributor Author

benoitc commented May 14, 2016

@uwiger The try/catch was mostly here to speed the usage, at least on erlang 18. Anyway I made the change you suggested. To skip the need to test the module each time it needed I create at compile time a module named jobs_time_compat. Let me know what you think about it.

@uwiger
Copy link
Owner

uwiger commented May 15, 2016

I think generating code is overkill for this particular problem. I would suggest going with an is_builtin() check, and the next optimization might be to generate a millisecond-timestamp directly, rather than a now()-tuple.

@uwiger
Copy link
Owner

uwiger commented May 15, 2016

... to clarify, with "next optimization", I didn't mean this PR. :)

replace erlang:now/0 by erlang:timestamp/0 on Erlang 18 and sup.
@benoitc
Copy link
Contributor Author

benoitc commented May 15, 2016

@uwiger just pushed a simpler version. Should be good now :)

@uwiger
Copy link
Owner

uwiger commented May 15, 2016

Did you push jobs_time_compat.erl?

@uwiger
Copy link
Owner

uwiger commented May 15, 2016

never mind - forgot to refresh.

@benoitc
Copy link
Contributor Author

benoitc commented May 15, 2016

? mm no I put time_compat/0 in jobs_lib right now

@benoitc
Copy link
Contributor Author

benoitc commented May 15, 2016

heh np :)

@uwiger uwiger merged commit 6317500 into uwiger:master May 15, 2016
@uwiger
Copy link
Owner

uwiger commented May 15, 2016

thanks!

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.

2 participants