Proposing: removing lines with more than one statements.

[Iximiel]

Description

Don't merge this right now.
I am starting from the v2.9 since v2.10 bumped from C++11 to C++17 and so I think that v2.9 will be maintained a little longer.

I am changing the astyle settings to have 1 line 1 operation, this should make the code more diff friendly and more readable.
This should also make the coverage by line more accurate

Another thing that this PR will help with is some comments like this:

atom_lab.resize(0); unsigned nnodes; // Delete all the atom labels that have been created

that (I think) they are not as clear as they should be:

// Delete all the atom labels that have been created
atom_lab.resize(0);
unsigned nnodes;

When everything is clear I will squash everything in a single commit and remove the "legacy option" from astyle

Target release

I would like my code to appear in release v2.9++

Type of contribution

• changes to code or doc authored by PLUMED developers, or additions of code in the core or within the default modules
• changes to a module not authored by you
• new module contribution or edit of a module authored by you

Copyright

• I agree to transfer the copyright of the code I have written to the PLUMED developers or to the author of the code I am modifying.

• the module I added or modified contains a COPYRIGHT file with the correct license information. Code should be released under an open source license. I also used the command cd src && ./header.sh mymodulename in order to make sure the headers of the module are correct.

Tests

• I added a new regtest or modified an existing regtest to validate my changes.
• I verified that all regtests are passed successfully on GitHub Actions.

[carlocamilloni]

I agree this way is more readible for both us and the the software tools

[Iximiel]

Another style question:
Do you prefer braces everywhere or not?
It looks like there is no uniform consensus along all Plumed, for example in a single function we can have:

void GridBase::addKernel( const KernelFunctions& kernel ) {
  plumed_dbg_assert( kernel.ndim()==dimension_ );
  std::vector<unsigned> nneighb=kernel.getSupport( dx_ );
  std::vector<index_t> neighbors=getNeighbors( kernel.getCenter(), nneighb );
  std::vector<double> xx( dimension_ );
  std::vector<std::unique_ptr<Value>> vv( dimension_ );
  std::string str_min, str_max;
  for(unsigned i=0; i<dimension_; ++i) {
    vv[i]=Tools::make_unique<Value>();
    if( pbc_[i] ) {
      Tools::convert(min_[i],str_min);
      Tools::convert(max_[i],str_max);
      vv[i]->setDomain( str_min, str_max );
    } else {
      vv[i]->setNotPeriodic();
    }
  }

// vv_ptr contains plain pointers obtained from vv.
// this is the simplest way to replace a unique_ptr here.
// perhaps the interface of kernel.evaluate() should be changed
// in order to accept a std::vector<std::unique_ptr<Value>>
  auto vv_ptr=Tools::unique2raw(vv);

  std::vector<double> der( dimension_ );
  for(unsigned i=0; i<neighbors.size(); ++i) {
    index_t ineigh=neighbors[i];
    getPoint( ineigh, xx );
    for(unsigned j=0; j<dimension_; ++j)
      vv[j]->set(xx[j]);
    double newval = kernel.evaluate( vv_ptr, der, usederiv_ );
    if( usederiv_ )
      addValueAndDerivatives( ineigh, newval, der );
    else
      addValue( ineigh, newval );
  }
}

where we have both a

    } else {
      vv[i]->setNotPeriodic();
    }

an one-line statement with braces, and a

    for(unsigned j=0; j<dimension_; ++j)
      vv[j]->set(xx[j]);
    double newval = kernel.evaluate( vv_ptr, der, usederiv_ );
    if( usederiv_ )
      addValueAndDerivatives( ineigh, newval, der );
    else
      addValue( ineigh, newval );

a series of one-line statements without braces

• changing to the braces is a matter of changing break-one-line-headers to add-braces in the astyle options
• changing to no braces means adding remove-braces to the astyle options

I prefer having the braces both for clarity and things like faster "printf-debugging" (since you will need to add the braces in case you'd want to add something to the cycle/branch anyway), and in any case in case you forgot them astyle will put them for you.

Having the braces, in my opinion, makes also nested for/if easier to understand

      for(unsigned j=0; j<dimension_; ++j) {
        fd[j]=D[j];
        for(unsigned i=0; i<dimension_; ++i)
          if(i!=j)
            fd[j]*=C[i];
      }

      for(unsigned j=0; j<dimension_; ++j) {
        fd[j]=D[j];
        for(unsigned i=0; i<dimension_; ++i) {
          if(i!=j) {
            fd[j]*=C[i];
          }
        }
      }

The compiler will not care but the reader (and future you) surely will

[GiovanniBussi]

I agree. I see that sometime having no newline can make the code more compact. However:

• it does not show untested code as such in coverage scan
• it makes the code less readable when there are multiple instructions in the same line (e.g. if(a) b=1; c=2;).

So I agree in having a new line after an if or for statement. As long as we have a new line, I think braces should be there for clarity. With more and more people using python daily, we need to avoid ambiguous code such as:

if(a)
    b=1;
    c=2;

[Iximiel]

The state is now far more strict than before:
The option that I did not manage to set up is to add brackets to consecutive nested branches/loops:
for example, this

for(int i=0; i<2; i++) for(int j=0; j<2; j++) for(int k=0; k<2; k++) shifts[i][j][k].clear();

became this

  for(int i=0; i<2; i++)
    for(int j=0; j<2; j++)
      for(int k=0; k<2; k++) {
        shifts[i][j][k].clear();
      }

but not this

  for(int i=0; i<2; i++) {
    for(int j=0; j<2; j++) {
      for(int k=0; k<2; k++) {
        shifts[i][j][k].clear();
      }
    }
  }

I do not think that astyle enforces this passage (at least, I am convinced about this because I did not find the option in the manual), but both versions pass the style check, and changing code manually is a long long manual work (that needs to be redone for 2.10, and master)

Still is clearer, and can be done atomically when someone edits that file and happens to remember that

Apart from that this is fine and ready
Then there is the merge to v2.10 (and to master) but with @GiovanniBussi we discussed how to do them

we'll need to open an issue or something like that for external contributors that are developing on older branches with the series of instructions like:

git merge hash-2.9-before-format
git merge -s ours hash-2.9-after-format
make astyle
git merge v2.9

[Iximiel]

Another thing that this PR will help with is some comments like this:

This sentence is not true anymore, to do that I would need to check more or less every file,
And since the merge strategy will be something like:

merge hash-before-this-pr
merge -s ours hash-of-this-pr
# some other commands in the middle
git checkout hash-of-this-pr .astyle.options
make astyle
# finalize the commit

All the comment editing will need to be redone by hand for each misplaced comment :/