-
-
Notifications
You must be signed in to change notification settings - Fork 118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix bug ci test 12 in ParMmg + improve consistencies of functions #226
Conversation
…+ improve comments + factorize code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks,
See my review ;-).
Best
src/mmg3d/split_3d.c
Outdated
|
||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As this deletion is useless, it introduces a diff with no reason: please, remove this modification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Sorry for that.
src/mmg3d/split_3d.c
Outdated
@@ -95,7 +93,7 @@ int MMG3D_split1_sim(MMG5_pMesh mesh,MMG5_pSol met,MMG5_int k,MMG5_int vx[6]) { | |||
MMG5_pTetra pt,pt0; | |||
double vold,vnew; | |||
uint8_t tau[4]; | |||
const uint8_t *taued; | |||
const uint8_t *taued=NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you adding this because your compiler raises a warning?
(In practice, taued will always be initialized even without the default affctation. The only case when it is not initialized in MMG3D_split*_cfg
is when element volume is to low and in this case taued will not be use as we will refuse the split.)
I find it more readable to not initialized it to NULL
(it means that it is always initialized in another location and that it has no sense to use this variable with a NULL value).
The same remark holds for other taued initializations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - I reversed this. I did that for consistency with other split functions initializing taued like this.
const int ne=3; | ||
|
||
pt[0] = &mesh->tetra[k]; | ||
flg = pt[0]->flag; | ||
pt[0]->flag = 0; | ||
newtet[0]=k; | ||
|
||
/* Determine tau, taued and imin the condition for vertices permutation */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment to explain that vGlobNum
is setted to pt->v
when we come from split2sf and this value may point to a wrong memory address if the tetra array is reallocated before the use of vGlobNum
(MMG3D_crea_newTetra
may reallocate the tetra array and vGlobNum
is used inside split2sf_cfg
) because the pt
pointer may become invalid. Thus, it is mandatory to call split*_cfg before newTetra function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
…of useless return;. Add comments to explain why MMG3D_split*_cfg should be before MMG3D_crea_newTetra.
Thanks! |
This PR fixes a bug in the ci test 12 in ParMmg when compile without SCOTCH library (Point 1). This PR also fixes some minor typos/style inconsistencies (Points 2-5) and factorizes some part of the code (Point 6).
In
src/mmg3d/split_3d.c
:MMG3D_split*_cfg
function before creation of new tetraMMG3D_crea_NewTetra
. This fixes the bug in ci test 12 of ParMmg.MMG3D_configSplit5
byMMG3D_split5_cfg
for name consistency with the otherMMG3D_split*_cfg
functions.return;
at the end of functionMMG3D_split1_cfg
,MMG3D_split3op_cfg
andMMG3D_update_qual
as they are defined asvoid
and thereturn;
is at the end of the function and does nothing (and for consistency with the otherMMG3D_split*_cfg
functions).taued
asNULL
in all functions.MMG3D_split3cone_sim
uses now directly the functionMMG3D_split3cone_cfg
.