Dubious code in Hydro_InitExcision.c

[Erik Schnetter]

Hydro_InitExcision.c contains the following code:

if ( (hydro_initexcision_coordinate_length <= 0.0) && ( ( x_frac > 0.5 - hydro_initexcision_fraction) && ( x_frac < 0.5 + hydro_initexcision_fraction) && ( y_frac > 0.5 - hydro_initexcision_fraction) && ( y_frac < 0.5 + hydro_initexcision_fraction) && ( z_frac > 0.5 - hydro_initexcision_fraction) && ( z_frac < 0.5 + hydro_initexcision_fraction) ) || ( (hydro_initexcision_coordinate_length > 0.0) && ( fabs(x[point]-hydro_initexcision_position_x) <= hydro_initexcision_coordinate_length*0.5) && ( fabs(y[point]-hydro_initexcision_position_y) <= hydro_initexcision_coordinate_length*0.5) && ( fabs(z[point]-hydro_initexcision_position_z) <= hydro_initexcision_coordinate_length*0.5) ) )

This code has an "and" (&&) and an "or" (||) operation at top level. Is this intended? The code would be clearer with an additional set of parenthesis, or by introducing a suitable set of temporaries for sub-expressions.

Keyword:

[Frank Löffler]

This code is not dubious to me. '&&' binds stronger than '| |', much like '*' binds stronger than '+'. I wouldn't see a problem with yet another set of parenthesis, but I also don't see the point of it. Please feel free to add more parenthesis if you like or maybe if some compiler thinks it is clever.

[Erik Schnetter]

Yes, I think people get confused with how strong && and || bind. However, I only wanted to check whether the expression is actually correct as coded.

... and if you want to remove superfluous parentheses, then I would begin with the ones surrounding the inequality comparisons.

[Erik Schnetter]

I attach a patch that cleans up the loop. It separates the logic of whether to excise from the statements that have to be executed to excise a grid point. It introduces local variables to simplify and document expressions. It also simplifies the loop structure.

Since all Hydro_InitExcision test cases are currently failing I could not test the patch.

[Frank Löffler]

Don't have the time to look into this now, and since it's close to the release and the change only cosmetical - delay it for the next release.

[Frank Löffler]

This patch lets the testsuites fail. Can you please try to reproduce the failure, and fix it if time permits?

[Frank Löffler]

Since the patch didn't work, I am removing the review state.