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

clang compatibility fixes #71

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

Conversation

kobigurk
Copy link
Contributor

This PR together with the one in libff has some solution for clang compatibility.

Specifically, it handles issues mentioned in #54 and #67, about the following:

  1. non-type template parameter deduction - by using a higher abstraction of the template parameter.
  2. UNUSED macro in libff.

@@ -60,8 +60,8 @@ struct knowledge_commitment {
template<typename T1, typename T2, mp_size_t m>
knowledge_commitment<T1,T2> operator*(const libff::bigint<m> &lhs, const knowledge_commitment<T1,T2> &rhs);

template<typename T1, typename T2, mp_size_t m, const libff::bigint<m> &modulus_p>
knowledge_commitment<T1,T2> operator*(const libff::Fp_model<m, modulus_p> &lhs, const knowledge_commitment<T1,T2> &rhs);
template<typename T1, typename T2, typename T>
Copy link
Member

@tromer tromer Mar 27, 2017

Choose a reason for hiding this comment

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

This less-specific templatization loses information, and could cause future problems overloading operator* for other types. The change assumes that anything you'd multiply by a knowledge_commitment would have an as_bigint() method.
Does the original exceed C++11, or is clang (and which version?) showing incomplete C+1+11 support here?

Copy link
Contributor Author

@kobigurk kobigurk Mar 27, 2017

Choose a reason for hiding this comment

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

Agree, this causes me to feel uneasiness as well, maybe we can figure out a compromise.
As far as it seems to me, it's a different interpretation regarding non-type template parameter deduction - gcc decides to infer "bigint" from "const bigint &" while clang is being more strict and decides not to.

This is a contained example that shows the same behavior:
http://coliru.stacked-crooked.com/a/583904c911c2e84b

@@ -69,8 +69,8 @@ knowledge_commitment<T1,T2> operator*(const libff::bigint<m> &lhs, const knowled
lhs * rhs.h);
}

template<typename T1, typename T2, mp_size_t m, const libff::bigint<m> &modulus_p>
knowledge_commitment<T1,T2> operator*(const libff::Fp_model<m, modulus_p> &lhs, const knowledge_commitment<T1,T2> &rhs)
template<typename T1, typename T2, typename T>
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@howardwu howardwu closed this May 29, 2017
@howardwu howardwu changed the base branch from cmake-master-merging to candidate-master June 7, 2017 05:30
@howardwu howardwu reopened this Jun 7, 2017
@howardwu howardwu closed this Aug 18, 2017
@howardwu howardwu changed the base branch from candidate-master to master August 18, 2017 02:24
@howardwu howardwu reopened this Aug 18, 2017
@copumpkin
Copy link

Any progress on this?

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.

4 participants