| Message ID | 20201013151241.3557005-9-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran Thanks for tidying this! On Tue, 13 Oct 2020 at 16:12, Kieran Bingham <kieran.bingham@ideasonboard.com> wrote: > > The function gauss_seidel2_SOR() makes use of a function scoped iterator > 'i', for several loops, and has a precedence of re-using the function > scoped iterator declaration, but then proceeds to alias the iterator > with a new declaration in one of the later loops. > > Re-use the existing iterator variable for consistency, and to prevent > variable aliasing. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > index 42fbc8a476ad..8447f8bec18b 100644 > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > @@ -628,7 +628,7 @@ static double gauss_seidel2_SOR(double const M[XY][4], double omega, > lambda[i] = compute_lambda_bottom(i, M, lambda); > lambda[0] = compute_lambda_bottom_start(0, M, lambda); > double max_diff = 0; > - for (int i = 0; i < XY; i++) { > + for (i = 0; i < XY; i++) { > lambda[i] = old_lambda[i] + (lambda[i] - old_lambda[i]) * omega; > if (fabs(lambda[i] - old_lambda[i]) > fabs(max_diff)) > max_diff = lambda[i] - old_lambda[i]; > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, On 2020-10-13 16:12:39 +0100, Kieran Bingham wrote: > The function gauss_seidel2_SOR() makes use of a function scoped iterator > 'i', for several loops, and has a precedence of re-using the function > scoped iterator declaration, but then proceeds to alias the iterator > with a new declaration in one of the later loops. > > Re-use the existing iterator variable for consistency, and to prevent > variable aliasing. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/ipa/raspberrypi/controller/rpi/alsc.cpp | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > index 42fbc8a476ad..8447f8bec18b 100644 > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp > @@ -628,7 +628,7 @@ static double gauss_seidel2_SOR(double const M[XY][4], double omega, > lambda[i] = compute_lambda_bottom(i, M, lambda); > lambda[0] = compute_lambda_bottom_start(0, M, lambda); > double max_diff = 0; > - for (int i = 0; i < XY; i++) { > + for (i = 0; i < XY; i++) { This change itself is good but reading the top of the function we have, static double gauss_seidel2_SOR(double const M[XY][4], double omega, double lambda[XY]) { double old_lambda[XY]; for (int i = 0; i < XY; i++) old_lambda[i] = lambda[i]; int i; Would it not make sens to also fix this up in this patch? > lambda[i] = old_lambda[i] + (lambda[i] - old_lambda[i]) * omega; > if (fabs(lambda[i] - old_lambda[i]) > fabs(max_diff)) > max_diff = lambda[i] - old_lambda[i]; > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On 14/10/2020 13:32, Niklas Söderlund wrote: > Hi Kieran, > > On 2020-10-13 16:12:39 +0100, Kieran Bingham wrote: >> The function gauss_seidel2_SOR() makes use of a function scoped iterator >> 'i', for several loops, and has a precedence of re-using the function >> scoped iterator declaration, but then proceeds to alias the iterator >> with a new declaration in one of the later loops. >> >> Re-use the existing iterator variable for consistency, and to prevent >> variable aliasing. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/ipa/raspberrypi/controller/rpi/alsc.cpp | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp >> index 42fbc8a476ad..8447f8bec18b 100644 >> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp >> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp >> @@ -628,7 +628,7 @@ static double gauss_seidel2_SOR(double const M[XY][4], double omega, >> lambda[i] = compute_lambda_bottom(i, M, lambda); >> lambda[0] = compute_lambda_bottom_start(0, M, lambda); >> double max_diff = 0; >> - for (int i = 0; i < XY; i++) { >> + for (i = 0; i < XY; i++) { > > This change itself is good but reading the top of the function we have, > > static double gauss_seidel2_SOR(double const M[XY][4], double omega, > double lambda[XY]) > { > double old_lambda[XY]; > for (int i = 0; i < XY; i++) > old_lambda[i] = lambda[i]; > int i; > > Would it not make sens to also fix this up in this patch? Ah, i'd missed that. The scoping means that there is no aliasing, so it didn't trigger the warning. I've updated the patch, thanks. -- Kieran > >> lambda[i] = old_lambda[i] + (lambda[i] - old_lambda[i]) * omega; >> if (fabs(lambda[i] - old_lambda[i]) > fabs(max_diff)) >> max_diff = lambda[i] - old_lambda[i]; >> -- >> 2.25.1 >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >
diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp index 42fbc8a476ad..8447f8bec18b 100644 --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp @@ -628,7 +628,7 @@ static double gauss_seidel2_SOR(double const M[XY][4], double omega, lambda[i] = compute_lambda_bottom(i, M, lambda); lambda[0] = compute_lambda_bottom_start(0, M, lambda); double max_diff = 0; - for (int i = 0; i < XY; i++) { + for (i = 0; i < XY; i++) { lambda[i] = old_lambda[i] + (lambda[i] - old_lambda[i]) * omega; if (fabs(lambda[i] - old_lambda[i]) > fabs(max_diff)) max_diff = lambda[i] - old_lambda[i];
The function gauss_seidel2_SOR() makes use of a function scoped iterator 'i', for several loops, and has a precedence of re-using the function scoped iterator declaration, but then proceeds to alias the iterator with a new declaration in one of the later loops. Re-use the existing iterator variable for consistency, and to prevent variable aliasing. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- src/ipa/raspberrypi/controller/rpi/alsc.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)