Menu
▾
▴
Re: [Stir-devel] code indentation
|
From: Thielemans, K. <k.t...@uc...> - 2016-04-06 13:00:50
|
Hi
Checking the commit lines only seems hard and in any case would end up with potentially very weird indentation. Checking the whole file is better but then results in both white-space and true changes in one commit. So, I think it is best to clean-up indentation of all code in one go, e.g. just after a release. Then we can tell everyone to use something like
git diff -b --ignore-blank-lines
when they want to compare to anything before that release. Obviously, it’d best to fix a tool and convention asap such that new code won’t be affected. (Matthias example seems to tell us that “indent” isn’t good enough really. I’d hate to have to introduce extra {} just to make “indent” happy.)
Obviously, this will still create trouble for branches. Not sure how to handle that. There’s some pointers at
http://stackoverflow.com/questions/5256769/git-merge-and-fixing-mixed-spaces-and-tabs-with-two-branches
As I said, help vastly appreciated!
Kris
From: David
Sent: 06 April 2016 02:22
To: Matthias J Ehrhardt <m.j...@da...>
Cc: sti...@li...
Subject: Re: [Stir-devel] code indentation
Kris,
Are you wanting to check the style of strictly the commit, or the file the change is committed on?
On Wed, Mar 30, 2016 at 6:21 AM, Matthias J Ehrhardt > wrote:
Hi there,
I am no expert in automatic code indentation but for some reason I found this interesting enough to spend an hour of mine. Please overlook if some of my comments are too basic ...
There seem to be some general decisions that we have to make in order to apply a coding style in STIR. As Kris pointed out, he doesn't like running a software on the whole code and change everything as this creates a lot of unnecessary history for STIR. Therefore, the other option to go for is probably to set up a pre-commit hook in GIT. This means every time you submit a file some code is executed before the actual commit. Here we could run some indentation software like "indent". The question is, do we just change the committed code to suit our standards or do we want flag that the code does not suit our standards and refuse the commit. Many companies seem to do the latter as this "teaches" the developers to follow their coding standards. For a project like STIR where non of us (except Kris maybe) is full time working on it, this seems like an overkill, so I would suggest we do the former.
As a starting point I wanted to try to automatically indent the file "RampFilter.cxx". There seems to be one initial problem.
[me404@login-sand7 FBP2D]$ indent -i2 -bl -bli0 RampFilter.cxx -o text.cxx
indent: RampFilter.cxx:67: Error:Unmatched 'else'
indent: RampFilter.cxx:72: Error:Unmatched 'else'
The file contains two if clauses that are not followed by brackets {}, see snippet:
{
const double x = n*2*fc;
// KT&Darren Hogg 17/05/2000 removed square(sampledist) as this introduced a scaling factor in the reconstructions
if (n==0)
return static_cast<float>((2*square(fc)*(-4 + alpha*(4 + square(_PI))))/(_PI/* *square(sampledist) */));
else if (fabs(fabs(x)-1) < 1E-6
return static_cast<float>(
-(square(2*fc)*(8*alpha + (-1 + alpha)*square(_PI)))/
(4*_PI/* *square(sampledist) */));
else
return static_cast<float>(
square(2*fc)*(
-alpha - square(x) + 3*alpha*square(x) - square(square(x)) +
_PI*x*sin(_PI*x)*(-1 + square(x))*
(-alpha + (-1 + 2*alpha)*square(x)) +
cos(_PI*x)*(alpha - (1 + alpha)*square(x) +
(-1 + 2*alpha)*square(square(x)))
)/
(_PI/* *square(sampledist) */*square(-1 + x)*square(x)*square(1 + x))
);
}
Fixing this as
{
const double x = n*2*fc;
// KT&Darren Hogg 17/05/2000 removed square(sampledist) as this introduced a scaling factor in the reconstructions
if (n==0) {
return
static_cast<float>((2*square(fc)*(-4 + alpha*(4 + square(_PI))))/(_PI/* *square(sampledist) */));
} else {if (fabs(fabs(x)-1) < 1E-6) {
return
static_cast<float>(
-(square(2*fc)*(8*alpha + (-1 + alpha)*square(_PI)))/
(4*_PI/* *square(sampledist) */));
} else {
return
static_cast<float>(
square(2*fc)*(
-alpha - square(x) + 3*alpha*square(x) - square(square(x)) +
_PI*x*sin(_PI*x)*(-1 + square(x))*
(-alpha + (-1 + 2*alpha)*square(x)) +
cos(_PI*x)*(alpha - (1 + alpha)*square(x) +
(-1 + 2*alpha)*square(square(x)))
)/
(_PI/* *square(sampledist) */*square(-1 + x)*square(x)*square(1 + x))
);
}
}
}
solves the problem.
Instead of understanding the syntax the software exited with an error message. After fixing this in the code, I could run the software and it gave me the output at the bottom. There are lots of changes. The tool "indent" gives me plenty of options to modify the style but we would need to agree on one. The few options I have chosen was to mimic Kris' snippet but it seems that this file was not using Kris' style anyhow. I would suggest we just use one of the predefined versions as it is just a convention anyway.
What are your thoughts on this?
regards,
Matthias
48d47
<
54,60c53,56
<
< static inline float
< ramp_filter_in_space(const int n,
< const float sampledist,
< const int length,
< const float alpha,
< const float fc)
---
> static inline float
> ramp_filter_in_space (const int n,
> const float sampledist,
> const int length, const float alpha, const float fc)
62c58
< const double x = n*2*fc;
---
> const double x = n * 2 * fc;
64,72c60,61
< if (n==0) {
< return
< static_cast<float>((2*square(fc)*(-4 + alpha*(4 + square(_PI))))/(_PI/* *square(sampledist) */));
< } else {if (fabs(fabs(x)-1) < 1E-6) {
< return
< static_cast<float>(
< -(square(2*fc)*(8*alpha + (-1 + alpha)*square(_PI)))/
< (4*_PI/* *square(sampledist) */));
< } else {
---
> if (n == 0)
> {
74,83c63,65
< static_cast<float>(
< square(2*fc)*(
< -alpha - square(x) + 3*alpha*square(x) - square(square(x)) +
< _PI*x*sin(_PI*x)*(-1 + square(x))*
< (-alpha + (-1 + 2*alpha)*square(x)) +
< cos(_PI*x)*(alpha - (1 + alpha)*square(x) +
< (-1 + 2*alpha)*square(square(x)))
< )/
< (_PI/* *square(sampledist) */*square(-1 + x)*square(x)*square(1 + x))
< );
---
> static_cast <
> float >((2 * square (fc) * (-4 + alpha * (4 + square (_PI)))) /
> (_PI /* *square(sampledist) */ ));
84a67,91
> else
> {
> if (fabs (fabs (x) - 1) < 1E-6)
> {
> return
> static_cast <
> float >(-(square (2 * fc) * (8 * alpha + (-1 + alpha) * square (_PI)))
> / (4 * _PI /* *square(sampledist) */ ));
> }
> else
> {
> return
> static_cast <
> float >(square (2 * fc) *
> (-alpha - square (x) + 3 * alpha * square (x) -
> square (square (x)) + _PI * x * sin (_PI * x) * (-1 +
> square (x))
> * (-alpha + (-1 + 2 * alpha) * square (x)) +
> cos (_PI * x) * (alpha - (1 + alpha) * square (x) +
> (-1 +
> 2 * alpha) * square (square (x)))) /
> (_PI /* *square(sampledist) */ * square (-1 + x) *
> square (x) * square (1 + x))
> );
> }
89,90c96,97
< RampFilter::RampFilter(float sampledist_v, int length , float alpha_v, float fc_v)
< :
---
> RampFilter::RampFilter (float sampledist_v, int length, float alpha_v,
> float fc_v):
92c99
< Filter1D <float>(length),
---
> Filter1D < float >(length),
94c101,103
< fc(std::min(fc_v, .5F)), alpha(alpha_v), sampledist(sampledist_v)
---
> fc (std::min (fc_v, .5F)),
> alpha (alpha_v),
> sampledist (sampledist_v)
97c106
< start_timers();
---
> start_timers ();
100c109
< if (length==0)
---
> if (length == 0)
108c117
< */
---
> */
116c125
< float f = 0.0;
---
> float f = 0.0;
118c127,128
< for (Int i = 1; i <= length - 1; i += 2) {
---
> for (Int i = 1; i <= length - 1; i += 2)
> {
120c130
< float nu_a = f ;
---
> float nu_a = f;
122c132
< filter[i] = nu_a * (alpha + (1. - alpha) * cos(_PI * f / fc));
---
> filter[i] = nu_a * (alpha + (1. - alpha) * cos (_PI * f / fc));
128c138
< filter[2] = (0.5) * (alpha + (1. - alpha) * cos(_PI * f / fc));
---
> filter[2] = (0.5) * (alpha + (1. - alpha) * cos (_PI * f / fc));
140c150
< */
---
> */
142c152
< assert(length%2==0);
---
> assert (length % 2 == 0);
146c156
< filter.set_offset(0);
---
> filter.set_offset (0);
148c158
< Array<1,float> filter(length);
---
> Array < 1, float >filter (length);
151c161
< filter[0] = ramp_filter_in_space(0, sampledist, length, alpha, fc);
---
> filter[0] = ramp_filter_in_space (0, sampledist, length, alpha, fc);
153c163
< for (int n = 1; n <= length/2; n += 1)
---
> for (int n = 1; n <= length / 2; n += 1)
155,156c165,166
< filter[n] = ramp_filter_in_space(n, sampledist, length, alpha, fc);
< filter[length-n] = filter[n];
---
> filter[n] = ramp_filter_in_space (n, sampledist, length, alpha, fc);
> filter[length - n] = filter[n];
161c171
< filter.set_offset(1);
---
> filter.set_offset (1);
163c173
< realft(filter, length/2, 1);
---
> realft (filter, length / 2, 1);
167,168c177,178
< if (set_kernel(filter) == Succeeded::no)
< error("Error initialisation ramp filter\n");
---
> if (set_kernel (filter) == Succeeded::no)
> error ("Error initialisation ramp filter\n");
173c183
< stop_timers();
---
> stop_timers ();
179c189
< std::string RampFilter:: parameter_info() const
---
> std::string RampFilter::parameter_info () constconst
183,184c193,196
< char str[1000];
< ostrstream s(str, 1000);
---
> char
> str[1000];
> ostrstream
> s (str, 1000);
187,189c199,200
< #endif
< s << "RampFilter :="
< << "\nFilter length := "
---
> #endif
> s << "RampFilter :=" << "\nFilter length := "
191c202
< << filter.get_length()
---
> << filter.get_length ()
193c204
< << (kernel_in_frequency_space.size()-1)*2
---
> << (kernel_in_frequency_space.size () - 1) * 2
195,199c206,209
< << "\nCut-off in cycles := "<< fc
< << "\nAlpha parameter := "<<alpha
< << "\nSample dist := "<< sampledist;
<
< return s.str();
---
> << "\nCut-off in cycles := " << fc
> << "\nAlpha parameter := " << alpha << "\nSample dist := " << sampledist;
>
> return s.str ();
[me404@login-sand7 FBP2D]$
On 29/03/2016 21:43, Thielemans, Kris wrote:
Hi
As we’re getting more contributions for STIR, it’s getting quite important of fix our indentation style, and have a recommended tool to enforce it before you commit a code-change. I’m looking for help here, to suggest the style, to find a tool and provide editor settings that enforce this.
An important consideration here is backwards compatibility. I don’t fancy having an automatic tool changing all of the STIR code (as git wouldn’t be terribly happy with that). We used emacs settings for this, but it looks like my emacs configuration settings have been lost after a number of HD crashes....
Most of STIR uses the following style:
while (x == y)
{
something();
somethingelse();
}
Using 2 spaces, tabs need to be expanded to spaces. No white-space at end of line.
These rules will probably cover most of it, but there’s probably more. I’d appreciate it of somebody could check what settings you’d need in emacs, then to see how this gets reproduced with a (portable) tool (such as GNU indent, or astyle), and then see if this leaves most of the STIR files unchanged.
Thanks
Kris
------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785471&iu=/4140
_______________________________________________
Stir-devel mailing list
Sti...@li...<mailto:Sti...@li...>
https://lists.sourceforge.net/lists/listinfo/stir-devel
--
Matthias Joachim Ehrhardt, PhD
Research Associate
Department for Applied Mathematics and Theoretical Physics, University of Cambridge
Centre for Mathematical Sciences, Wilberforce Road, Cambridge CB3 0WA
m.j...@da...<mailto:m.j...@da...>
------------------------------------------------------------------------------
Transform Data into Opportunity.
Accelerate data analysis in your applications with
Intel Data Analytics Acceleration Library.
Click to learn more.
http://pubads.g.doubleclick.net/gampad/clk?id=278785471&iu=/4140
_______________________________________________
Stir-devel mailing list
Sti...@li...<mailto:Sti...@li...>
https://lists.sourceforge.net/lists/listinfo/stir-devel
|
