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

Optimize Keccak implementation and remove Crypto++ dependency #47

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

chfast
Copy link
Member

@chfast chfast commented Apr 29, 2015

As I talked with @LefterisJP and @gavofyork some time ago this is some code from my pet project https://github.com/chfast/ethminer.

Review on Reviewable

#define FOR5(v, s, e) v = 0; REPEAT5(e; v += s;)

/// Keccak-f[1600]
inline void keccakf1600(uint64_t* a) noexcept
Copy link
Contributor

Choose a reason for hiding this comment

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

This fails on Windows since msvc seems to not havenoexcept.
Can you add something like:

#ifndef _MSC_VER
#define NOEXCEPT noexcept
#else
#define NOEXCEPT
#endif

and use that instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not needed in the function? So you mean you will remove the attribute in general?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep. Compiler will know that without my help.

@LefterisJP
Copy link
Contributor

Can you also update python's setup.py?
https://github.com/ethereum/ethash/blob/master/setup.py

sha3.c would need to be renamed to keccak.cpp there too.
You can see that travis fails there on the python build.

@chriseth
Copy link
Contributor

Some license headers are missing.
Can we re-license "CC0, attribution kindly requested, by external author" as GPL?

@chfast
Copy link
Member Author

chfast commented Apr 29, 2015

Not sure about CC0 - GPL compatibility.

@gavofyork
Copy link
Contributor

fails on the CI?

@LefterisJP
Copy link
Contributor

Hmmm .. this will also need rebasing now. As for the CI it's the python tests that fail since python's setup.py needs updating after the file changes.

@chfast
Copy link
Member Author

chfast commented May 5, 2015

CI fails is because my added file is C++11 and requires -std=c++11 flag and the rest is C that requires -std=gnu99. Python module setup script do not allow you to set separated flags for C and C++.

The solution might be to restrict implementation to C or C++ only.

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

Successfully merging this pull request may close these issues.

4 participants