Coding style of avrdude

[Ho-Ro]

During the modification of avrdude I stumbled upon many differently formatted source code files - you can easily see the long history of this project. The most obvious thing for me was the indentation, 2 spaces vs. 4 spaces (sometimes even mixed in one file, e.g. main.c), sometimes hard tabs, but also a lot of trailing spaces and empty lines full of spaces here and there.
Also different function definition style

int
function(int a, int b)
{
    ...
}

vs.

int function(int a, int b) {
    ...
}

So the question I have is whether you avrdudes have a preferred coding style, e.g. for creating new source code files? I did not find any hint, also not in discussions. If nothing is carved in stone, what is accepted, where is the red line?
For example, I prefer a very speaking "airy " style with lots of whitespace around the operators and the brackets and aligned comments, supported by my clang-format settings:

int doSomethingSpecial( int sensorInput, int parameter ) { // keep temperature below ...
   if ( sensorInput >= aGlobalLimit ) {                   // too hot ...
       parameter = 2 * parameter + anotherConstant;       // increase the cooling flow ...
       ...
   }
}

vs.

int f(int a, int b)
{
    if (a>=123) {
        b=2*b+4321;
        ...
    }
}

OTAH a no-go for me are trailing spaces in code, my normal editor setting (as well as my clang-format default) is to remove all trailing whitespace in source code automatically when saving the file. For avrdude edits I have switched off this function because there are so many superfluous spaces, if you collected them all and printed them out, you would have stationery for many years :)

In one of my bigger projects (Qt based), that was forked from an abandoned old project, I did an industrial cleaning once - no code changes, just reformatting according my coding style. This was one monster commit, but then everything was consistent. Next step was squashing the hundreds of warnings that QtCreator yelled at me.

What is your opinion? Should avrdude also get a session at the beauty parlour? Maybe not for 7.3, but 8.0 could be a target?
Please tell me if this is a stupid idea! (I put my thoughts by purpose in this section "Chat about anything and everything here" :)

Martin

[dl8dtl]

Yes, there have been many different contributors, with many different styles in the past.

Well, the first and foremost rule: if you are touching an existing file, try to align with the style you can already find there. This also means: as annoying as trailing white space is (I completely agree), do not be tempted to mangle with it unless you are touching the respective code area anyway.

The reason for that rule is very simple: if you later review a diff across such a change, the white-space only changes could easily obfuscate the actual code changes.

Spaces after opening and before closing parentheses are not commonly used in most coding guidelines, and I think AVRDUDE code has not been using it so far – so better avoid that. For whatever historical reasons, the rule of thumb is to have a space before the opening parenthesis in an operator context (for, while, if) but not to have one in a function call context. Binary operators should always have spaces, so "a + b", not "a+b". It improves readability, and in some situations (negative sign with numbers) might even disambiguate the grammar (consider something like "a+-5", vs. "a + -5").

Do not use "Hungarian" notation in any way. If you need a global variable, rather consider prefixing it with a kind of module name or such, like "stk500_is_initialized" rather than "gIsInitialized". I think most readers prefer underscore_style over CamelCase.

But that's it: just my opinions.

[mcuee]

I think this is quite a good guideline.

[Ho-Ro]

Jörg,
Thanks, your message could go directly into Coding_Style.md almost unchanged. I checked my previous commits and saw that I have mostly followed these rules in the past, also out of respect for the original authors.
Martin

EDIT: The .editorconfig in the top directory of the repo gives also a good guideline, indent_size = 4 is a good trade-off between space-saving and readability - if you look hours on source code, 2 spaces is too stressful. Wider indent serves also an educational purpose, with the reduction of indented program levels you automatically reduce the complexity of the code.

# EditorConfig: https://editorconfig.org

root = true

...

[*.{c,h,cpp,y}]
tab_width = 4
indent_size = 4
indent_style = space
insert_final_newline = true
trim_trailing_whitespace = true
´´´

[stefanrueger]

Here a few tips for coding style (my tuppence):

• Use two-space indentations and, in particular, do no not use tabs
• Declare pointers as char *p and not char* p
• Make use of const declarations for function pointer arguments when the function only reads from the object pointed at
• Put one-line comments like so // Test (note the capital T and the space after //)
• If possible, place right-of-code comments at column 32 (counting from 0)
• Make use of empty lines when you want to separate operational blocks (personal judgement)
• Open brace after program control, not in new line, eg, use if(x) {
• Avoid unnecessary operations, eg, prefer return x & 1; over return !!(x & 1) == 1; or return x & 1? 1: 0;
• Use short but meaningful names, eg, prefer status over resp_status_code or s
• Do no brace single-commands, ie, prefer

  if(error) 
    return -1;
  

  over

  if(error) {
    return -1;
  } 
  

• Do not else a return, ie, prefer

  if(error)
    return -1;
  do_this();
  do_that();
  

  over

  if(error) {
    return -1;
  } else {
    do_this();
    do_that();
  }
  

• Be brief, eg, prefer

  return status == S_ACK? 0: status == S_NAK? 1: -1;
  

  over

  if (status == S_ACK)
    return 0;
  if (status == S_NAK)
    return 1;
  return -1;
  

• Declare variables close to where they are used, eg, prefer

  for(int i=0; i<42; i++) {
    // ...
  }
  

  over

  int i;
  // ...
  for(i=0; i<42; i++) {
  }  
  

These are meant to be examples for the guiding principles below and not as must dos.

[MCUdude]

Great summary @stefanrueger! Lots of code are more or less formatted this way, so my suggestion would be to use this as a reference guide. However, I'm not a fan of automatic spell/formatting checking. Sometimes you'd like to do things differently for specific reasons, and automated spell/formatting checking wouldn't like that.

[stefanrueger]

Most contributors will have their own programming style. AVRDUDE is relatively liberal in that the project has a history of accepting longer stand-alone pieces in the "handwriting" of the author. I actually find it hard to write code in a different style than the one that flows naturally as writing in a different style would be too much distraction for me. I personally have two guiding principles: a) the complexity of the code should be commensurate with the complexity of the underlying problem b) tasks should be decomposed into smaller ones that fit a screen, so that they can be read without scrolling.

Both principles help maintenance and readability.

I realise that the size of a screen/window has changed dramatically over the last decades, but something like

  int n;

  if(something)
  {
    n = 0;
  }
    else
  {
    n  = 1;
  }

violates both principles: 10 lines of code for what can simply be expressed as int n = !something;

I personally am partial to compact writing and using tables in code (as this often helps reigning in complexity).

[dl8dtl]

I agree, even though I think it's a little simplified for an example (won't work that easily e.g. if you want n to be 42 instead).
Not requiring braces for very simple if/then statements has always been something I voted for, also in style guides for commercial entities I've been involved.

    if (something)
        n = 42;
    else
        n = 13;

If it helps making things simple and clear, I'm even in favor of the ternary operator.

   return status == OK? 0: -1;

[stefanrueger]

won't work that easily e.g. if you want n to be 42 instead

True, though I have been known to write, eg, int n = 42 * !something; (I imagine that modern processors often produce faster and more compact code when not needing to clear the pipeline for branching, though a good compiler can reduce branching on its own account). I defo wouldn't go as far as int n = 42 + (13-42)*!something; as it's hard to read, but int n = something? 42: 13; wins hands down in my world.

Ultimately, it comes down to preference and taste. Luckily, the AVRDUDE project (and I) can cope with many a style.

[dl8dtl]

• Be brief, eg, prefer
  return status == S_ACK? 0: status == S_NAK? 1: -1;

I actually find that hard to read.

I'm really a fan of the ternary operator where it improves readability, but even after decades of C programming, I would have to think about that construct, rather than being able to "just read" it.

Other than really simple cases, I would restrict the use of the ternary operator to situations like macros where all has to fit into a single line. (But even then, one might consider an inline function instead.)

[stefanrueger]

return status == S_ACK? 0: status == S_NAK? 1: -1;

I read this as Return 0 if status is ack, 1 if status is nack, and -1 otherwise.

Once you see the pattern, even a long chain $t_1$? $e_1$: $t_2$? $e_2$: $\ldots\ t_n$? $e_n$: $e_0$ is easy to read: it evaluates to $e_1$ if $t_1$ holds, $e_2$ if $t_2$ holds, $\ldots e_n$ if $t_n$ holds, and $e_0$ failing that. Chained conditional operators have neat properties:

• Compact
• Lazy: evaluation stops with first true $t_w$; in that sense it's like $t_1$ || $t_2$ || $\ldots$ || $t_n$
• Exactly one of the $e_i$ expressions is evaluated: the one $e_w$ belonging to the first true test $t_w$ or, failing that, $e_0$
• Order matters, but if at most one $t_i$ can be true the conditions can be reordered so that the most probable is tested first
• Only one return or assignment needs be coded rather than $n+1$ ones
• Even complex tests and expressions can be neatly formatted using $n+2$ lines rather than $2n+2$ ones:

  result = 
    complex_test(1)? complex_expression(1):
    complex_test(2)? complex_expression(2):
    // and so on
    complex_test(n)? complex_expression(n):
    failing_all_expression();
  

As an aside, there can be real advantages from expression computation over flow of control changes: branchless computations can be quicker owing to CPU architectures with pipelines. Whilst $t?\ e_1{:}\ e_0$ does resort to branching, it lends itself to be transformed into a branchless computation, eg, $t \cdot e_1 + (1 - t) \cdot e_0$ assuming $t \in \{0, 1\}$. As an added bonus, the latter has constant computation time (important for cryptographic applications where one doesn't want to give away the state of computed bits by the execution time).