Skip to content
New issue

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

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

Already on GitHub? # to your account

Use BitOp or Bit32 libraries where available #2

Merged
merged 2 commits into from
Feb 9, 2015
Merged

Use BitOp or Bit32 libraries where available #2

merged 2 commits into from
Feb 9, 2015

Conversation

pablomayobre
Copy link
Contributor

Tested and got between 300 and 500 faster performance which I consider a HUGE amount.

I dont know why the commit looks so horrible but I just made a few minor changes

@kikito
Copy link
Owner

kikito commented Feb 8, 2015

Hi Positive07,

Have you tested this in systems without the "bit" library? As far as I know, this line should fail:

if require "bit" or require "bit32" then

When you try to require a library which does not exist, it does not return false - it raises an error. You should be able to reproduce it by switching the order in the if: if require 'bit32' or require 'bit' then.

The appropriate way of doing this should involve pcall:

local ok, bit = pcall(require, 'bit')
if not ok then ok, bit = pcall(require, 'bit32') end
if ok then
  bit_or, bit_and, bit_not, bit_xor = bit.bor, bit.band, bit.bnot, bit.bxor
  bit_rshift, bit_lshift = bit.rshift, bit.lshift
else
  ...

I would do the changes myself, but I am travelling and can't do that at the moment. Can you add the change and make sure it works when the libraries are not present?

Added pcalls instead of raw requires as suggested by Kikito
@pablomayobre
Copy link
Contributor Author

Thanks Kikito, you where right! it errored right away, fixed it as you suggested, I'll test this fix in a couple of hours and come back to confirm that it works

@pablomayobre
Copy link
Contributor Author

It works perfectly fine now

kikito added a commit that referenced this pull request Feb 9, 2015
Use BitOp or Bit32 libraries where available
@kikito kikito merged commit b56a8a1 into kikito:master Feb 9, 2015
@kikito
Copy link
Owner

kikito commented Feb 9, 2015

Thanks a lot, merged!

@kikito
Copy link
Owner

kikito commented Feb 9, 2015

I have setup md5.lua in Travis so that tests can be run automatically in the three main envs (Lua 5.1, 5.2 & LuaJIT). This should make changes easier in the future.

I have also included you in the credits readme, released version 1.0.0 of the library, and published it in Luarocks. Thanks again!

@pablomayobre
Copy link
Contributor Author

Wow amazing! Thank you!

@lxss
Copy link

lxss commented Apr 5, 2015

md5.lua Less than 824 bytes, MD5 is correct
If more than 823 bytes,md5 is wrong!
I am a novice, I can't find the reason.

@kikito
Copy link
Owner

kikito commented Apr 5, 2015

Hi @lxss, please create a new issue instead of commenting on a closed one.

@lxss
Copy link

lxss commented Apr 5, 2015

I'm sorry, I have just registered, don't know how to use

require "md5"
local md5 = require 'md5'

local str = "";
for i=1,823 do
str = str .. "1";
end
print(md5.sumhexa(str)); --823 correct
str = str .. "1";
print(md5.sumhexa(str)); --824 error

@kikito
Copy link
Owner

kikito commented Apr 5, 2015

@lxss I have created an issue for you here: #4

I also have a couple questions, please answer them there.

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

Successfully merging this pull request may close these issues.

3 participants