fmihpc / vlasiator

Vlasiator - ten letters you can count on
https://www.helsinki.fi/en/researchgroups/vlasiator
Other
44 stars 35 forks source link

Compilation warnings: misleading indentation in Dispersion #238

Open iljah opened 8 years ago

iljah commented 8 years ago

Same setup as in #235, here's an example of new functionality in GCC:

projects/Dispersion/Dispersion.cpp:147:13: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
             creal result = avg *
             ^~~~~
projects/Dispersion/Dispersion.cpp:137:10: note: ...this 'for' clause, but it is not
          for (uint vj=0; vj<this->nVelocitySamples; ++vj)
          ^~~

result in https://github.com/fmihpc/vlasiator/blob/master/projects/Dispersion/Dispersion.cpp#L147 is only executed once as fas as I can see

ykempf commented 8 years ago

The (triple) for loop is closed by the single curly braces, this is fully intentional.

ykempf commented 8 years ago

However the block after the for loops should be two levels back. Either it was badly formatted or a clever automatic indentation tool in some text editor fooled me.

iljah commented 8 years ago

But they stop before result is assigned, also intentional? Similar thing in https://github.com/fmihpc/vlasiator/blob/master/projects/Harris/Harris.cpp#L90 and in https://github.com/fmihpc/vlasiator/blob/master/projects/Magnetosphere/Magnetosphere.cpp#L184 and in https://github.com/fmihpc/vlasiator/blob/master/projects/Riemann1/Riemann1.cpp#L108

ykempf commented 8 years ago

Huh? Returns after the curly braces which close the for loop stack. Perfectly fine code. One /could/ pove the block back 5 levels but it'd look weird. I fashioned it this way since the for loops are one-liners.

ykempf commented 8 years ago

But if you have time to spare/waste on fixing these two, please do. If I touch the code there some time I might remember to do it. These work as intended so it's not a problem. If your futuristic compiler finds actual bugs due to misleading indentation (we have had some, so it's well possible), then let us know however.

iljah commented 8 years ago

Currently the code looks as if the thing which gcc complains about would be executed more than one time. If the code is supposed to be outside of for loops why is it indented as if it should be inside? I'll document all the occuring instances before fixing them...

ykempf commented 8 years ago

There is curly braces however to avoid the reader being misled. I agree the indentation is not as clean as could be, but this is not high on my task list at the moment.

galfthan commented 8 years ago

As far as I can see it is intentional and code is correct. To be honest there are a few inconsistencies:

ykempf commented 8 years ago

The line is commented out, I didn't even see it. ;) This looks like a remnant of actually 2D simulations, but in any case could/should be taken out.

galfthan commented 8 years ago

The easiest way to fix this, and other indentation & style issues is to create an astyle with the correct style for Vlasiator, and run it on the whole source code.