Dubious code in Hydro_InitExcision.c

Issue #222 resolved
Erik Schnetter created an issue

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:

Comments (10)

  1. Frank Löffler
    • removed comment

    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.

  2. Erik Schnetter reporter
    • removed comment

    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.

  3. Erik Schnetter reporter
    • removed comment

    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.

  4. 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.

  5. Frank Löffler
    • assigned issue to
    • removed comment

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

  6. Roland Haas
    • edited description
    • changed status to resolved

    The parenthesis warnings were addressed in git hash 6a533323 "Hydro_InitExcision: place parentheses better (and thus silence compiler warnings)" of einsteininitialdata on Thu Nov 15 04:35:46 2012 +0000 and the code compiles without warnings.

    The proposed patch contained other changes that are unrelated to this. If desired to have them included, please rebase and re-propose.

  7. Log in to comment