C++ coding standards and code quality suggestions

HarryR commented 6 years ago

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 commented 6 years ago

I've added three automated code quality checkers:

drewstone commented 6 years ago

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:

gitcoinbot commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

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:

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 commented 5 years ago

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 commented 5 years ago

I would recommend the following document for guideline.

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

gitcoinbot commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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

jcastle13 commented 5 years ago

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

HarryR commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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:

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 commented 5 years ago

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

jcastle13 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago

Hi @aman935 and @upendra2412

Have you any suggestions about improving code quality?

aman935 commented 5 years ago

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

HarryR commented 5 years ago

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 commented 5 years ago

@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

gitcoinbot commented 5 years ago

@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

aman935 commented 5 years ago

Here are the changes, I will suggest. Many of these are very small ones, I will add more PRs later for the purpose:

Code on a line should be limited to 80 chars, this improves human readability of the code. stackoverflow, Google Style Guide
Indentation is not strict. Found to be 8 spaces one some pages and 4 on some. Please choose whichever you want to enforce. Also, the google style guide says that it should be 2(but, it says a lot of other things also and is very strict about them). I would suggest making it 4.

I would add other suggestions and make improvement as I further learn about them. Please help me in implementing these suggestions on the files that haven't been updated yet(too many files). For now, I have created a PR implementing part of the 1st suggestion. Will keep adding more as I get time for it.

gitcoinbot commented 5 years ago

@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] reminder (3 days)
[ ] escalation to mods (6 days)

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

gitcoinbot commented 5 years ago

@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] reminder (3 days)
[ ] escalation to mods (6 days)

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

gitcoinbot commented 5 years ago

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

@aman935 due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

[x] reminder (3 days)
[x] escalation to mods (6 days)

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

gitcoinbot commented 5 years ago

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

@aman935 due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

[x] reminder (3 days)
[x] escalation to mods (6 days)

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

aman935 commented 5 years ago

@HarryR . I tried generating the report. I installed pvs-studio on my system and tried to run it to produce the report. There is a problem here. The pvs-studio report cannot be generated unless the project builds successfully. And the build status is failing, as of now. Setting up pvs-studio is very simple. Also, if you use visual studio, pvs-studio report can also be generated using its plugin there. So, will you please let me know, how the project successfully builds, or what is lacking? From my side, when I try to run with cmake, it gives me CmakeLists.txt error. that some .cpp file is not found.

gitcoinbot commented 5 years ago

@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] reminder (3 days)
[ ] escalation to mods (6 days)

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

gitcoinbot commented 5 years ago

@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] reminder (3 days)
[ ] escalation to mods (6 days)

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

gitcoinbot commented 5 years ago

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

@aman935 due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

[x] reminder (3 days)
[x] escalation to mods (6 days)

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

gitcoinbot commented 5 years ago

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

@aman935 due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

[x] reminder (3 days)
[x] escalation to mods (6 days)

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

rmshea commented 5 years ago

Hey @aman935, what's the latest here? Ryan from Gitcoin checking in 😄

rmshea commented 5 years ago

@aman935 how's it going?

HarryR / ethsnarks

C++ coding standards and code quality suggestions #18