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

atom.c: Initial demonstration of combining variable definition and initialization #439

Closed
wants to merge 2 commits into from

Conversation

schwehr
Copy link
Collaborator

@schwehr schwehr commented Sep 13, 2023

No description provided.

@derobins derobins added Component - C Library Core C library issues Type - Improvement Improvements that don't add a new feature or functionality Priority - 3. Low 🔽 Code cleanup, small feature change requests, etc. labels Sep 13, 2023
@derobins
Copy link
Member

derobins commented Sep 13, 2023

We've talked about doing something like this in HDF5, but have decided to stick with the 'function scope variables at the top' scheme there and we should probably keep that style here, too.

Historically, this was obviously due to C89 requirements, particularly MSVC. There are other key reasons to keep the existing scheme, though:

  • The number 1 reason IMHO is that it makes it a LOT easier to look at a function and be sure that you are cleaning up allocated resources properly. In HDF5, incorrect resource cleanup has been a frequent source of bugs, especially in error conditions. Putting everything at the top, initializing pointers, etc. to "bad" values like NULL, and then making sure that everything allocated is cleaned up at the end has been a solid paradigm for the library. Spreading declarations throughout a function makes this harder.
  • Also, it would take a lot of work to refactor the entire library to the new style. This would probably take a long time and the library would have an ugly mixed style while that work was in progress.

I know there are good reasons for making this sort of change, but I think in our case, being super careful about resource allocation and cleanup trumps other style arguments.

I could maybe be convinced otherwise, but I'm probably going to decline this shift in style, at least for now.

@schwehr
Copy link
Collaborator Author

schwehr commented Sep 14, 2023

No worries. I've typically found the opposite. It's harder to keep things straight for humans when they have wider scope and tightening up things allows better analysis and the additions of const. Doing this kind of thing for GDAL helped me get rid of a ton of lingering issues with error cases, resource allocation, and the like.

I will try a couple different takes on parts of this idea to see if they work for you. All this the goal of reducing complexity & risk and hopefully improving readability.

@schwehr schwehr closed this Sep 14, 2023
@derobins
Copy link
Member

Yeah, I can probably be convinced otherwise. I just know that we have more problems in HDF5 with proper error cleanup than we do with using uninitialized values, etc.

I think one key thing, regardless of how variables are declared, is refactoring large functions. I'm hoping that once we get the code into a more refactorable state we can make progress there and then to-may-to / to-mah-to decisions are less impactful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - C Library Core C library issues 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.

2 participants