erlyaws / yaws

Yaws webserver
https://erlyaws.github.io
BSD 3-Clause "New" or "Revised" License
1.29k stars 267 forks source link

Replaced deprecated crypto:rand_uniform/2 with rand:uniform/1. #302

Closed ningzhang closed 7 years ago

ningzhang commented 7 years ago

When I tried to compile Yaws with the latest Erlang (built from source), I got this error:

yaws_ctl.erl:65: crypto:rand_uniform/2 is deprecated and will be removed in a future release; use rand:uniform/1

So, I replaced:

crypto:rand_uniform(Lo, Hi) -> N , Lo =< N < Hi

with

rand:uniform(N) -> X , 1 <= X <= N

capflam commented 7 years ago

Hi,

Rand module was introduced in Erlang/OTP release 18. So for all previous releases, the rand/0 function will throw an exception. This is not a big deal because of the try...catch.... But, I think, the best would be to rely on yaws_dynopts module in all cases. So the function can be simplified as following:

rand() ->
    {A1, A2, A3}=yaws:get_time_tuple(),
    yaws_dynopts:random_seed(A1, A2, A3),
    yaws_dynopts:random_uniform(1 bsl 64).

To be credited for the patch, could you amend you pull request ?

Thanks!

ningzhang commented 7 years ago

Hi Christopher,

Thanks for the reply. I kind of understand what you mean, but I don't know exactly how to do it. Could you please come up with a new patch and trash mine. I'll watch and learn this time.

Cheers!

ningzhang commented 7 years ago

It's scary to touch something that I appreciate very much but don't fully understand. ><

capflam commented 7 years ago

Well, If it's easier for you, you can open a new pull request and close this one. That's not a problem. There is always a beginning for everything :) But if you don't feel comfortable, I can get your pull request manually and amend it by hand. That's not a problem.

ningzhang commented 7 years ago

Do you mean to replace this entire function:

rand() ->
    case os:type() of
        {win32, _} ->
            {A1, A2, A3}=yaws:get_time_tuple(),
            yaws_dynopts:random_seed(A1, A2, A3),
            yaws_dynopts:random_uniform(1 bsl 64);
        _ ->
            try
                rand:uniform(1 bsl 64) - 1
            catch
                _:_ ->
                    error_logger:warning_msg("Running without crypto app\n"),
                    {A1, A2, A3}=yaws:get_time_tuple(),
                    yaws_dynopts:random_seed(A1, A2, A3),
                    yaws_dynopts:random_uniform(1 bsl 64)
            end
    end.

with

rand() ->
    {A1, A2, A3}=yaws:get_time_tuple(),
    yaws_dynopts:random_seed(A1, A2, A3),
    yaws_dynopts:random_uniform(1 bsl 64).

If so, I think I can manage to do that with pleasure.

capflam commented 7 years ago

Yes, that's it.

ningzhang commented 7 years ago

Okay, I got it. Thanks:) One more question. I noticed that we have mixed style of A=B and A = B in Yaws. Should I prefer one over the other when committing?

capflam commented 7 years ago

Well, there is not really a coding style for Yaws. Personally, I prefer A = B. But, as you can see, I'm not always consistent with my opinion :). Honestly, if the code is readable, it's ok for me.

ningzhang commented 7 years ago

I got it. Thanks for your time, Christopher. I will be closing this pull request and initiate a new one shortly after.