Skip to content

Fix bug ci test 12 in ParMmg + improve consistencies of functions#226

Merged
Algiane merged 4 commits intoMmgTools:developfrom
laetitia-m:feature/pmmg-bug-test12
Nov 9, 2023
Merged

Fix bug ci test 12 in ParMmg + improve consistencies of functions#226
Algiane merged 4 commits intoMmgTools:developfrom
laetitia-m:feature/pmmg-bug-test12

Conversation

@laetitia-m
Copy link
Copy Markdown
Contributor

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:

  1. Move MMG3D_split*_cfg function before creation of new tetra MMG3D_crea_NewTetra. This fixes the bug in ci test 12 of ParMmg.
  2. Rename MMG3D_configSplit5 by MMG3D_split5_cfg for name consistency with the other MMG3D_split*_cfg functions.
  3. Remove return; at the end of function MMG3D_split1_cfg, MMG3D_split3op_cfg and MMG3D_update_qual as they are defined as void and the return; is at the end of the function and does nothing (and for consistency with the other MMG3D_split*_cfg functions).
  4. Initialized taued as NULL in all functions.
  5. Improve some comments.
  6. In MMG3D_split3cone_sim uses now directly the function MMG3D_split3cone_cfg.

Copy link
Copy Markdown
Member

@Algiane Algiane left a 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

Comment on lines -80 to -81

return;
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Sorry for that.

double vold,vnew;
uint8_t tau[4];
const uint8_t *taued;
const uint8_t *taued=NULL;
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

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.

pt[0]->flag = 0;
newtet[0]=k;

/* Determine tau, taued and imin the condition for vertices permutation */
Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Contributor Author

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.
@Algiane Algiane merged commit 9f3a06d into MmgTools:develop Nov 9, 2023
@Algiane
Copy link
Copy Markdown
Member

Algiane commented Nov 9, 2023

Thanks!

@laetitia-m laetitia-m deleted the feature/pmmg-bug-test12 branch November 9, 2023 11:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants