moved long function definitions out of Gmsh class declaration and header file#429
moved long function definitions out of Gmsh class declaration and header file#429meng5113 wants to merge 1 commit intosolvcon:masterfrom
Conversation
|
@j8xixo12 could you please help take a look too? |
yungyuc
left a comment
yungyuc
left a comment
There was a problem hiding this comment.
Thanks, @meng5113 . Please follow the directives of the linter to fix the issues: https://github.com/solvcon/modmesh/actions/runs/11191984882/job/31118518269?pr=429#step:11:389 . You can turn on Github Actions in your forked repository to get the same results.
| } | ||
|
|
||
| // Check the finite state machine state transition is valid or not to check msh file format is correct | ||
| bool Gmsh::is_valid_transition(const std::string s) |
There was a problem hiding this comment.
Could you please help change the argument to be a const reference? It's a simple change but needs to be applied in both the declaration (in header file) and the definition (here).
To be specific, it is to change
bool Gmsh::is_valid_transition(const std::string s)with
bool Gmsh::is_valid_transition(const std::string & s)That is, change the argument of the function from const std::string s to const std::string & s.
The change will speed up the code by saving unnecessary string copying.
| { | ||
| return s == "$MeshFormat"; | ||
| } | ||
| else if (last_fmt_state == FormatState::META_END || last_fmt_state == FormatState::PYHSICAL_NAME_END) |
There was a problem hiding this comment.
It looks odd to me that you use "or" here. Why not split them into two else if?
There was a problem hiding this comment.
This is my bad, I have checked the gmsh manual section 10.3.1.
According to the manual, after MeshFormat, which is the META_END state, it can either be followed by $PhysicalNames or $Nodes. That is why the function returns an "or" expression. However the problem is that after physical name section, it only can be followed by Node. Therefore this code should be changed to
...
else if (last_fmt_state == FormatState::META_END)
{
return s == "$PhysicalNames" || s == "$Nodes";
}
else if (last_fmt_state == FormatState::PYHSICAL_NAME_END)
{
return s == "$Nodes";
}
...
Thanks @tigercosmos to point out this problem.
I think we can use another PR to fix this problem, it should add some test case to make sure this logic is correct!
I will file an issue to track this.
| return false; | ||
| } | ||
|
|
||
| void Gmsh::load_meta(void) |
| msh_ver = std::stod(tokens[0]); | ||
|
|
||
| // The parse only support ver 2.2 msh file. | ||
| if (msh_ver != 2.2) |
There was a problem hiding this comment.
I will choose to use constexpr for the number.
There was a problem hiding this comment.
Why not just use a macro ?
There was a problem hiding this comment.
Macro does not allow compilers to check type. When macros and constexpr both work, choose constexpr.
There was a problem hiding this comment.
For version number, integer should be used. Floating could be inexact and should be avoided.
| m_nds(std::stoul(tokens[0]) - 1, 2) = std::stod(tokens[3]); | ||
| } | ||
|
|
||
| if (!line.compare("$EndNodes")) |
There was a problem hiding this comment.
I think you should maintain a table to keep these token strings.
| } | ||
| usnds.insert(usnds.end(), nds_temp.begin() + 1, nds_temp.end()); | ||
| nds_temp[0] = mmcl.size(); | ||
| m_elems.insert(std::pair{idx, nds_temp}); |
There was a problem hiding this comment.
I think emplace will be better?
| } | ||
|
|
||
| bool is_valid_transition(const std::string s); | ||
| void load_meta(void); |
There was a problem hiding this comment.
No need to have void in the argument.
4 participants
The PR is to move long function definitions out of Gmsh class declaration and header file in issue #425.