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

hdf/test: Remove local prototypes by moving local only functions to before use #445

Closed
wants to merge 1 commit into from

Conversation

schwehr
Copy link
Collaborator

@schwehr schwehr commented Sep 15, 2023

This gets rid of a few redundant prototypes.

…before use

This gets rid of a few redundant prototypes.
@derobins
Copy link
Member

This is another one of those changes I'm on the fence about. Is this really better? In HDF5 we got sick of fragile function ordering and mandated function prototypes. In the tests it's probably okay since they are usually only used in the main function, but I'm not a hurry to go back to having to reorder unrelated code because I moved something. Maintaining the prototypes also sucks. I'm just not sure which one sucks more. Thoughts?

@bmribler
Copy link
Collaborator

bmribler commented Sep 15, 2023

"...having to reorder unrelated code because I moved something..."
My opinion, this one is worse.

@schwehr
Copy link
Collaborator Author

schwehr commented Sep 15, 2023

For this type of stuff, I'm strongly in favor of removing them. The extra stuff adds up. Especially in testing, if there is an order dependency issue showing up, the compiler will complain and it frequently indicates a bug. Here for example, there is one prototype that I had to keep and it indicates there there is problem with a missing function in the headers for the tests.

@bmribler bmribler added Component - Testing Test code, GitHub workflows Type - Improvement Improvements that don't add a new feature or functionality Priority - 3. Low 🔽 Code cleanup, small feature change requests, etc. labels Sep 18, 2023
@bmribler
Copy link
Collaborator

bmribler commented Sep 19, 2023

a missing function in the headers for the tests.

Could you give me more info on this missing function?

@schwehr
Copy link
Collaborator Author

schwehr commented Sep 19, 2023

a missing function in the headers for the tests.

Could you give me more info on this missing function?

This:

extern void test_mgr_dup_images();

extern void test_mgr_dup_images();

should be in a header as the function is actually here:

test_mgr_dup_images()

@bmribler
Copy link
Collaborator

bmribler commented Sep 19, 2023

a missing function in the headers for the tests.

Could you give me more info on this missing function?

This:

extern void test_mgr_dup_images();

extern void test_mgr_dup_images();

should be in a header as the function is actually here:

https://github.com/HDFGroup/hdf4/blob/bb23516f05e764db2e8eeec59a429f1180bbe309/hdf/test/tdupimgs.c

You're right, it should have been in tproto.h, my mistake. I'll fix it.

@schwehr schwehr changed the title hdf/test: Remove local prototypess by moving local only functions to before use hdf/test: Remove local prototypes by moving local only functions to before use Sep 29, 2023
@schwehr
Copy link
Collaborator Author

schwehr commented Sep 29, 2023

As discussed out-of-band, this PR does not match the style the HDF4 team wants, so I'm closing the PR.

@schwehr schwehr closed this Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Testing Test code, GitHub workflows Priority - 3. Low 🔽 Code cleanup, small feature change requests, etc. Type - Improvement Improvements that don't add a new feature or functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants