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

K&R calma/** #332

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

dlmiles
Copy link
Contributor

@dlmiles dlmiles commented Oct 5, 2024

This series converts the calma module into ANSI function prototype syntax.

Please feedback if the method and commit breakdown helps the review process (for example github has single character change highlight, the code style adopted is to help get through review, a future commit could make just style cleanup/white-space only change which can be reviewed better via diff -b)

There are 2 subsequent change set behind this one (calma/** .rodata cleanup, ATTR_FORMAT_PRINTF - reminder to self)

dlmiles added 14 commits October 4, 2024 11:18
Rename existing prototype method name to correct error.

K&R obsolete syntax removal for C23 compatibility series
K&R obsolete syntax removal for C23 compatibility series
K&R obsolete syntax removal for C23 compatibility series
Remove forward declaration prototype to correct error.

K&R obsolete syntax removal for C23 compatibility series
Remove forward declaration prototype to correct error.

K&R obsolete syntax removal for C23 compatibility series
time.h has existed since C89 so is a standard header expected
to always be available.

sys/time.h was an optional header that historically only some
platforms provided.

If there is a conflict on specific platforms it is better to
'#if !defined()' that specific niche platform with the problem
if both headers are included in the same compile unit.  But I
don't think this is a problem in modern times.

So this results in a resolution that removes #ifdef around
time.h and the detection by configure for the availabiltiy
of sys/time.h.

K&R obsolete syntax removal for C23 compatibility series
K&R obsolete syntax removal for C23 compatibility series
K&R obsolete syntax removal for C23 compatibility series
K&R obsolete syntax removal for C23 compatibility series
K&R obsolete syntax removal for C23 compatibility series
K&R obsolete syntax removal for C23 compatibility series
K&R obsolete syntax removal for C23 compatibility series
K&R obsolete syntax removal for C23 compatibility series
@RTimothyEdwards
Copy link
Owner

Is this supposed to be a preview change for converting all of the subroutines to have ANSI function prototypes? Am I supposed to merge this one or just merge them all at once?

@dlmiles
Copy link
Contributor Author

dlmiles commented Oct 10, 2024

Yes this can merge, if there are no major or minor objections.

If its in a PR and it builds ok, and there is no quality warning in PR comment, assume it is ready. GH has a draft PR function to try to prevent not-ready change-sets from being merged, but still allow review to occur.

I try to make the PR a complete merge-able standalone unit.

The 2 change sets behind this one means I have not queued 2 other PRs up on Github because they have an order dependency on this PR (because the same lines or nearby lines are changed, for another reason). They are temporarily withheld to prevent the headache of merge conflicts.

The K&R will be a slow burning matter over time, so expect a bunch, then a gap, then a bunch, etc.. i've a few more directories in alphabetical order then knee deep into 'database' but I think that will end up last as the callback cleanups are quite expansive and need a few cycles of review (on my side) to be ready.

@RTimothyEdwards
Copy link
Owner

I think it's best if we don't make too many changes at a time. Finish up with the various other issues that don't have to do with ANSI declarations, and when that seems to have settled down, we can start applying these.

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