C++ coding standards and code quality suggestions

[HarryR]

We need to slowly improve the quality of the C++ code used in the ethsnarks project.

Any insight from skilled C++ practitioners is welcome, especially if it provides an example of before & after, and how this improves code quality etc.

For example:

• namespaces aren't used for ethsnarks code, but should be
• CMake files could be cleaner
• common patterns could be made into composable units/templates
• anti-patterns and unnecessary duplication
• C++11 functionality which can be taken advantage of
• reducing memory usage, avoiding copy constructors

[HarryR]

I've added three automated code quality checkers:

• https://app.codacy.com/project/HarryR/ethsnarks/dashboard
• https://bettercodehub.com/results/HarryR/ethsnarks
• https://www.codefactor.io/repository/github/harryr/ethsnarks

[drewstone]

Do you have any style guidelines/links that you are currently following? As I'm learning c++, I'm interested in starting to familiarize myself with proper standards. Here are some below if you have some in mind:

• Google's c++ style guide
• Bjarne Stroustrup C++ Style and Technique FAQ
• Cpp Core Guidelines

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

This issue now has a funding of 80.0 DAI (80.0 USD @ $1.0/DAI) attached to it.

• If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
• Want to chip in? Add your own contribution here.
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $20,513.54 more funded OSS Work available on the Gitcoin Issue Explorer

[HarryR]

Hi @drewstone

I'm not following any specific C++ style, but I've reviewed some of the guidelines and I'm slowly making progress. I've been trying to make the C++ easier to read, and easier to modify, namely using namespaces and typedefs, e.g. instead of libsnark::protoboard<FieldT> and libsnark::pb_variable<FieldT> there is now ProtoboardT and VariableT

additionally most things are under the ethsnarks namespace, with many files split into implementation and header.

More details in https://github.com/HarryR/ethsnarks/compare/master...HarryR:cxx-cleanups-plus-merkle?expand=1

Generally things I try to do are:

• Initialise all variables at constructor time, following a strong RAII pattern
• Use const whenever possible

For example, with the RAII pattern, if you have a gadget which has an output variable - don't require the output variable to be passed as an argument - the gadget owns the output variable and must be accessed via the .result() method. This makes composing multiple gadgets together easier, as there is no need for the composer to setup variables to handle the output - the result of one gadget is passed directly as the input to another.

However, I'll spend some more time and make efforts towards cleaner code.

But... if you have some input, especially where things just don't make sense from a programmers perspective, and specific examples of how to sort improve them (and pointing out specific parts of good C++ references / stye guides which justify that approach) then there's a gitcoin bounty for this which you could easily grab.

Template for new gadgets:

#include "ethsnarks.hpp"

namespace ethsnarks {
class myexample_gadget : public GadgetT
protected:
    const VariableT &my_input;
    VariableT my_output;
public:
    myexample_gadget(
        ProtoboardT &in_pb,
        const VariableT &in_input,
        const std::string in_annotation_prefix=""
    ) :
        GadgetT(in_pb, in_annotation_prefix),
        my_input(in_input),
        my_output( make_variable(in_pb, "my_output") )
    { ... }
}

One of the problems is, for example, the pb_variable constructor doesn't allow you to give it the protoboard, and needs another step for allocate, so I made a make_variable function which does this.

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

Workers have applied to start work.

These users each claimed they can complete the work by 11 months, 2 weeks from now. Please review their action plans below:

1. jcastle13 has applied to start work (Funders only: approve worker | reject worker).

   I would like to work on this project if it’s not have been completed already. I feel that, with my experience in C/C++, I would be able to contribute on some advice and code refactoring based on some key design patterns and templates.

Learn more on the Gitcoin Issue Details page.

[HarryR]

Hi @jcastle13

I've made some improvements on this branch: https://github.com/HarryR/ethsnarks/tree/cxx-cleanups-plus-merkle

Any insight into how to make it more difficult to introduce bugs or make accidental mistakes, make it easier for developers to understand, or any common practices which would help to establish a solid and easy-to-contribute-to codebase are appreciated.

Although I noticed you haven't got much C++ on your profile, and we need to wait for the funder (@ vs77bb) to approve stuff.

One thing I noticed is that the VariableT (pb_variable<T>) type is just a wrapper around an int, so passing by reference can introduce subtle data-passing bugs.

[jcastle13]

I would recommend the following document for guideline.

https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md

[gitcoinbot]

@jcastle13 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] warning (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[gitcoinbot]

@jcastle13 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] warning (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[jcastle13]

I've been a bit busy but would still like to contribute. I tried to look for the branch ( https://github.com/HarryR/ethsnarks/tree/cxx-cleanups-plus-merkle) but was unable to locate it. I get a 404 when clicking on the link. Has it moved?

[HarryR]

@jcastle13 sorry, I merged that branch into master. See: https://github.com/HarryR/ethsnarks

[jcastle13]

Cool. I’ll check it out. Should have some time during the end of the week.

[HarryR]

Awesome, thanks for spending the time to help.

Are there any projects you've come across with very clean C++ programming practices, that would be good to use for positive influence and where lessons can be learned to help this project?

[gitcoinbot]

@jcastle13 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] warning (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[gitcoinbot]

@jcastle13 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] warning (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[jcastle13]

@HarryR I'm sorry but I've been swamped on my current project which actually happens to be purely on C++11. Since I've haven't had time to work on the project this past week I will drop but if I can contribute in the future I will.

[HarryR]

No problem @jcastle13 - that's just an automated message from gitco.in (I have no control over it).

At the end of the day this is all about learning how to write better more robust code.

[jcastle13]

@HarryR Sounds good. In that case I'll contribute whenever time permits. I'm assuming that I can clone the master branch if need be or is there another branch that has been forked?

[HarryR]

Ya you can clone master, feel free to make big breaking changes as long as the automated builds pass and we slowly improve the tests, there are also a handful of other related projects which could do with attention, depending on what interests you:

• https://github.com/JacobEberhardt/ZoKrates/
• https://github.com/barryWhiteHat/roll_up
• https://github.com/iden3/circom

etc.

There are lots of C++11 features which make life easier, such as the new for syntax, and rvalue, auto types, default constructors (e.g. int x{5} in class). But are they applied consistently and usefully throughout the project, and does it really make life simpler more secure and easier for people to understand with less room for error?

[mkosowsk]

Hi @jcastle13, have you had a chance to take a look at this issue in recent days? Thanks! 👍

[jcastle13]

Hello @mkosowsk. Unfortunately I have not had a chance to look into the issue in recent days. I've been pretty busy lately. If time permits, I will definitely try to look into it.

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

Workers have applied to start work.

These users each claimed they can complete the work by 10 months, 1 week from now. Please review their action plans below:

1) pandyamarut has applied to start work (Funders only: approve worker | reject worker).

I'll follow the contributing guidelines and showcase my relevant skills.

Learn more on the Gitcoin Issue Details page.

[mkosowsk]

@pandyamarut feel free to start work! I am having some issues approving you on the back-end but you should be good to go 👍

[gitcoinbot]

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

---

Workers have applied to start work.

These users each claimed they can complete the work by 10 months from now. Please review their action plans below:

1) upendra2412 has applied to start work (Funders only: approve worker | reject worker).

With my experience in C++, will do most of the refactoring as possible

Learn more on the Gitcoin Issue Details page.

[gitcoinbot]

@upendra2412 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] warning (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[gitcoinbot]

@aman935 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] warning (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

[HarryR]

Hi @aman935 and @upendra2412

Have you any suggestions about improving code quality?

[aman935]

Hi @HarryR I have suggestions, should I simply put them here? Or in some README, and then push the PR?

[HarryR]

I guess it depends on what they are. You could post them here, or create tickets which describe them, or submit a pull request with improvements.

[gitcoinbot]

@aman935 Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

• [x] warning (3 days)
• [ ] escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days