#222: Dubious code in Hydro_InitExcision.c ----------------------+----------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: new Priority: minor | Milestone: Component: Cactus | Version: Keywords: | ----------------------+----------------------------------------------------- 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.
#222: Dubious code in Hydro_InitExcision.c -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: new Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: -----------------------+----------------------------------------------------
Comment (by knarf):
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.
#222: Dubious code in Hydro_InitExcision.c -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: new Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: -----------------------+----------------------------------------------------
Comment (by eschnett):
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.
#222: Dubious code in Hydro_InitExcision.c -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: new Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: -----------------------+----------------------------------------------------
Comment (by eschnett):
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.
#222: Dubious code in Hydro_InitExcision.c -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: new Priority: minor | Milestone: ET_2011_05 Component: Cactus | Version: Resolution: | Keywords: -----------------------+---------------------------------------------------- Changes (by knarf):
* milestone: => ET_2011_05
#222: Dubious code in Hydro_InitExcision.c -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: review Priority: minor | Milestone: ET_2011_05 Component: Cactus | Version: Resolution: | Keywords: -----------------------+---------------------------------------------------- Changes (by eschnett):
* status: new => review
#222: Dubious code in Hydro_InitExcision.c -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: Type: defect | Status: review Priority: minor | Milestone: ET_2011_11 Component: Cactus | Version: Resolution: | Keywords: -----------------------+---------------------------------------------------- Changes (by knarf):
* milestone: ET_2011_05 => ET_2011_11
Comment:
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.
#222: Dubious code in Hydro_InitExcision.c -----------------------+---------------------------------------------------- Reporter: eschnett | Owner: eschnett Type: defect | Status: review Priority: minor | Milestone: ET_2011_11 Component: Cactus | Version: Resolution: | Keywords: -----------------------+---------------------------------------------------- Changes (by knarf):
* owner: => eschnett
Comment:
This patch lets the testsuites fail. Can you please try to reproduce the failure, and fix it if time permits?
#222: Dubious code in Hydro_InitExcision.c --------------------------+------------------------------------------------- Reporter: eschnett | Owner: eschnett Type: enhancement | Status: review Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: --------------------------+------------------------------------------------- Changes (by knarf):
* type: defect => enhancement * milestone: ET_2011_11 =>
#222: Dubious code in Hydro_InitExcision.c --------------------------+------------------------------------------------- Reporter: eschnett | Owner: eschnett Type: enhancement | Status: reopened Priority: minor | Milestone: Component: Cactus | Version: Resolution: | Keywords: --------------------------+------------------------------------------------- Changes (by knarf):
* status: review => reopened
Comment:
Since the patch didn't work, I am removing the review state.
trac@lists.einsteintoolkit.org
-
Einstein Toolkit