Modify

Opened 8 years ago

Last modified 7 years ago

#222 reopened enhancement

Dubious code in Hydro_InitExcision.c

Reported by: Erik Schnetter Owned by: Erik Schnetter
Priority: minor Milestone:
Component: Cactus Version:
Keywords: Cc:

Description

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.

Attachments (1)

Hydro_InitExcision.diff (12.4 KB) - added by Erik Schnetter 8 years ago.
patch

Download all attachments as: .zip

Change History (10)

comment:1 Changed 8 years ago by 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.

Last edited 8 years ago by Frank Löffler (previous) (diff)

comment:2 Changed 8 years ago by 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.

comment:3 Changed 8 years ago by 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.

Changed 8 years ago by Erik Schnetter

Attachment: Hydro_InitExcision.diff added

patch

comment:4 Changed 8 years ago by Frank Löffler

Milestone: ET_2011_05

comment:5 Changed 8 years ago by Erik Schnetter

Status: newreview

comment:6 Changed 8 years ago by Frank Löffler

Milestone: ET_2011_05ET_2011_11

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.

comment:7 Changed 7 years ago by Frank Löffler

Owner: set to Erik Schnetter

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

comment:8 Changed 7 years ago by Frank Löffler

Milestone: ET_2011_11
Type: defectenhancement

comment:9 Changed 7 years ago by Frank Löffler

Status: reviewreopened

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

Modify Ticket

Change Properties
Set your email in Preferences
Action
as reopened The owner will remain Erik Schnetter.
Next status will be 'review'.
as The resolution will be set.
to The owner will be changed from Erik Schnetter to the specified user.
The owner will be changed from Erik Schnetter to anonymous.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.