iotaledger / access-server

Apache License 2.0
14 stars 3 forks source link

Code guidelines #99

Closed djordjeglbvc closed 4 years ago

djordjeglbvc commented 4 years ago

Summary Define code guidelines to be used across Access project code tree.

djordjeglbvc commented 4 years ago

@bernardoaraujor After reviewing GNU and linux kernel coding standards (interesting to us, as they are both lowercase), I think it would best fit access to use modified GNU code style. Modifications would be:

  1. Indentation size is 4 spaces, instead of 2 defined in standard
  2. Don't use additional indentation for curly braces

To illustrate second point, acording to GNU standard, 'if' block would look something like this (which I think looks ugly):

if (something)
  {
    do_something();
  }

and with two modifications proposed above, it would look like this (as it already does):

if (something)
{
    do_something();
}

We would then convert all uppercase to lowercase in module prefix parts of symbol names (functions, types...).

As a note, wallet module uses modified kernel style:

  1. Indentation size is 2 spaces, instead of 8 defined in standard
  2. In standard, curly braces after function declaration go to their own new line, here they are in the same line as declaration

Links / references https://www.kernel.org/doc/html/latest/process/coding-style.html https://www.gnu.org/prep/standards/html_node/Writing-C.html

bernardoaraujor commented 4 years ago

sounds good @djordjeglbvc , I agree that the proposed modifications make the code look better.

Please check clang-format tool. I think we can use it to automate the checking.

But we should investigate how to do the proposed modifications to GNU. Probably something like this:

$ clang-format -style=gnu -dump-config > .clang-format

Then, edit .clang-format file to introduce the modifications to GNU style.

After that, the tool can be used via:

$ clang-format -style=file

But I'm also open to suggestions. I don't have any particular preference to GNU, I just chose because of it's influence in Linux. Here is a list of alternatives: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#configurable-format-style-options

djordjeglbvc commented 4 years ago

@bernardoaraujor I think GNU style is best starting point for us.

I've added few changes to dumped config options from clang-format command which I think would suit us best:

AlwaysBreakAfterDefinitionReturnType: None
AlwaysBreakAfterReturnType: None
BraceWrapping:
  IndentBraces: false
BreakBeforeBraces: Custom
IndentCaseLabels: true
IndentWidth: 4
SpaceBeforeParens: ControlStatements

If this is ok, I can add code style guidelines to readme (or someplace else), and fix the code. I would also add a detail about naming convention regarding globally visible names - adding module prefix. This isn't checked by tool, it would be our responsibility to do it manually.

strahinjagolic commented 4 years ago

@djordjeglbvc If I may suggest, we should also leave return type of the function in the same line with function's name and not as suggested in GNU coding style:

static char concat (char s1, char *s2) { … }

djordjeglbvc commented 4 years ago

@strahinjagolic yes, it is what these two do:

AlwaysBreakAfterDefinitionReturnType: None
AlwaysBreakAfterReturnType: None
bernardoaraujor commented 4 years ago

thanks guys!

@djordjeglbvc please commit your .clang-format to the project root so we can use it on a regular basis.

djordjeglbvc commented 4 years ago

@bernardoaraujor sure, I am preparing commit with .clang-format and fixes across all code in repository if that is ok. And one more question, where should I put code style guidelines doc, perhaps create access/blob/specs_docs/docs/04-code_guidelines.md?

bernardoaraujor commented 4 years ago

@bernardoaraujor sure, I am preparing commit with .clang-format and fixes across all code in repository if that is ok.

That's awesome thanks!

And one more question, where should I put code style guidelines doc, perhaps create access/blob/specs_docs/docs/04-code_guidelines.md?

Not necessary, I already placed it here: https://github.com/iotaledger/access/blob/specs_docs/specs/3-SPECS/access-ENGINEERING-SPEC-0000.md#programming-language

djordjeglbvc commented 4 years ago

@bernardoaraujor Updated list of changes from GNU defaults:

AlwaysBreakAfterDefinitionReturnType: None
AlwaysBreakAfterReturnType: None
BraceWrapping:
  AfterExternBlock: false
  IndentBraces: false
BreakBeforeBraces: Custom
BreakStringLiterals: false
ColumnLimit: 128
IndentCaseLabels: true
IndentWidth:     4
SpaceBeforeParens: ControlStatements

access and plugins directories are not done here, @strahinjagolic is working on them on separate branch.

clang-format has some limitations - for instance it doesn't check case style, that needs to be done manually. And option 'AfterExternBlock' requires at least clang-format-8, otherwise it indents everything within extern "C" {} block.

@oopsmonk please take a look at 217feabaca2443df2f4474c6be07e51114939f3d, these are the changes made to code in wallet directory only by clang-format using .clang-format file included at code_style_guidelines branch. If you have complaints, suggestions or otherwise.

strahinjagolic commented 4 years ago

Access and plugin done in separate branch

oopsmonk commented 4 years ago

@djordjeglbvc I think it's better to create a PR containing the .clang-format only for style discussion then apply the style once it's finalized so we can keep a clear history of this repository. for automation, we can set up git hooks to validate style before sources commit to the local and server.

it's a good time for case styles, camelCase and snake_case(iota.c) are commonly used. I don't have strong opinions on styles LLVM, Google, and GNU are commonly used in C, but I'd like to keep IndentWidth with 2 because 4 occupies more space and increases line breaks.

btw, wallet module uses Google-style, the default style could be different depends on clang-format version. https://github.com/iotaledger/iota.c/blob/master/.clang-format

djordjeglbvc commented 4 years ago

@oopsmonk then it makes perfect sense to also use that format in access repository. I'll change that, create pull request with .clang-format used on iota.c, then proceed with fixing formatting.

edit: pull request #103

bernardoaraujor commented 4 years ago

closed by PR #104