Skip to content
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

Cleaning Cppcheck warnings and refactor tests #115

Merged
merged 15 commits into from
May 14, 2023
Merged

Cleaning Cppcheck warnings and refactor tests #115

merged 15 commits into from
May 14, 2023

Conversation

scemama
Copy link
Member

@scemama scemama commented Mar 24, 2023

Pointers

instead of const char my_string[256] = "hello", we should use const char* my_string = "hello"

index_p

I created a const and a non_const variable. If a new array is allocated, we use index_p and free the non_const variable. If we can use the initial variable, index_p is NULL and we don't use the non_const variable.

String overflow

If a struct has a fixed-size string + some data, making an overflow in the string will corrupt data of the struct. So it is recommended to put strings at the end of structs, so that important data are less likely to be overwritten by s string overflow.

@scemama
Copy link
Member Author

scemama commented Mar 24, 2023

Pointers

instead of const char my_string[256] = "hello", we should use const char* my_string = "hello"

index_p

I created a const and a non_const variable. If a new array is allocated, we use index_p and free the non_const variable. If we can use the initial variable, index_p is NULL and we don't use the non_const variable.

String overflow

If a struct has a fixed-size string + some data, making an overflow in the string will corrupt data of the struct. So it is recommended to put strings at the end of structs, so that important data are less likely to be overwritten by s string overflow.

Copy link
Member

@q-posev q-posev left a comment

Choose a reason for hiding this comment

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

The string changes are very useful, thanks!

@scemama
Copy link
Member Author

scemama commented Apr 3, 2023

@q-posev ping!

@q-posev
Copy link
Member

q-posev commented Apr 3, 2023

@q-posev ping!

@scemama I responded a few days ago in this thread. I still believe that changes in this PR break the I/O of indices in 32-bit, which is why I am not ready to merge it.

Copy link
Member

@q-posev q-posev left a comment

Choose a reason for hiding this comment

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

@scemama Sorry, the review was marked as pending for some reason by GitHub... You should be able to see it now

src/templates_hdf5/templator_hdf5.org Show resolved Hide resolved
src/templates_hdf5/templator_hdf5.org Show resolved Hide resolved
@q-posev
Copy link
Member

q-posev commented Apr 12, 2023

@scemama ping 🙂

@q-posev
Copy link
Member

q-posev commented May 3, 2023

@scemama thanks! I think the name of the PR can be changed to reflect that there was a major refactoring of the unit tests. I have one comment, let me know if you prefer to get it fixed in a separate PR or in this one:

We discussed testing the io_dset_sparse with different values of mo_num in order to test at least the 8/16-bit and 32-bit code paths.

@scemama scemama changed the title Cleaning Cppcheck warnings Cleaning Cppcheck warnings and refactor tests May 3, 2023
@scemama
Copy link
Member Author

scemama commented May 3, 2023

Yes! It is better if I fix the tests in this PR also. I planned to do it, but I forgot...

@scemama
Copy link
Member Author

scemama commented May 13, 2023

We discussed testing the io_dset_sparse with different values of mo_num in order to test at least the 8/16-bit and 32-bit code paths.

Done!

@q-posev q-posev merged commit b51896e into master May 14, 2023
@scemama scemama deleted the cppcheck branch May 14, 2023 10:42
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