Minor edits for C best practices

[kiranshila]

Labels at the end of compound statements

Some switch statements have trailing labels with no statements, which is technically deprecated because of compatibility reasons with C++

Unused Parameters

Understanding that some arguments are purposefully unused to maintain API compat with the upstream library, we should mark them as such (not just in docs) so the compiler doesn't complain. GCC (and Clang) has extensions for this, MSVC does not. It seems the "best" way to accomplish this is with (void) unused;

Standard Compliance

I've added compiler checks to guarantee C99 compliance. Along with that comes the requirement that we define M_PI as a standards-conforming math.h actually must not define M_PI by default.

Initializers

In ISO C, empty initializers ({}) are forbidden. The “universal” initializer is {0} as in ISO C (at least until C23), an initializer list must contain at least one element.

Switch Case Fall-through

One common lint is to warn on switch case fall-thoughs, which would easily be a bug in some cases. Here, all the cases were commented, but that doesn't convince the compiler. In GCC there is __attribute__ ((fallthrough)) but it isn't portable. The only thing we can do to convince ourselves (and at least some compilers) is to reword the comment such that GCC knows what we mean.

If we want to convince more compilers, we can borrow from the linux kernel and do this

[attipaci]

@kiranshila, OK I'm learning new things every day :-). Let me know when it's ready to review and merge.

[attipaci]

I think it's OK for the refraction models to not use all the arguments that it might use. They are provided in case the model needs them as an input. Currently, none of the built-in refraction models use jd_tt, but you could construct your own refraction model that uses historical or forecast data, which is time-dependent. Hence, the parameter is supplied to allow such time-dependent refraction models being integrated.

[kiranshila]

I think it's OK for the refraction models to not use all the arguments that it might use.

Makes sense! I'd just want to document that so it's clear that that is intentional. I'll do that here.

[attipaci]

I'm trying to find some documentation on C++ needing a statement on default. So far I came up empy handed. If you could point me to somewhere where it's discussed, I'd be grateful. It's strange that cppcheck did not complain, even though it should check for validity of C++ syntax...

[kiranshila]

I'm trying to find some documentation on C++ needing a statement on default. So far I came up empy handed. If you could point me to somewhere where it's discussed, I'd be grateful. It's strange that cppcheck did not complain, even though it should check for validity of C++ syntax...

I believe it's quite recent in C++:

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2324r0.pdf

[attipaci]

I believe it's quite recent in C++:

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2324r0.pdf

Hmm, I'm not entirely convinced that this applies to switch statements per se. I think it applies to labels being applied in general (otherwise, why not use a switch block as an example instead of a function block?). My guess is that it was always OK inside a switch, but it may be worth checking with an older and newer C++ compiler if they barf either way... But I'm also OK with adding the ; as it does not harm either...

[attipaci]

Looking good so far @kiranshila. I'll sign off until tomorrow. It's getting late in on the old Continent...

[attipaci]

Is it in response to a compiler warning? Otherwise, I'm not sure this is necessary. You are correct that the initializer needs to cover at least one element (although gcc seems to not require even that), but after the defined elements, all the remaining footprint of the structure is initialized to 0, as if by memset(). So, there should be no need to explicitly define null substructures for the initializer. I think it's just unnecessarily verbose, but I might be wrong...

[kiranshila]

This one is tricky - I try to lean heavy into the "explicit is better than implicit camp", and -Wextra does as well - throwing a warning on partial initialization. You'd think we could just slap a {0} at the end and call it a day, but apparently there is an old GCC bug about that? https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119

But I agree it is a little on the extra verbose side - although I've been mulling about the design of object a bit still. It would be better if it weren't possible to construct an invalid object at all - meaning a sidereal object must contain a valid cat_entry while ephem stuff must not contain it, at a type level. I don't think that's possible in C, though.

[kiranshila]

This is all of course my opinion, and you are welcome to cherry-pick any of this stuff haha

[attipaci]

Yes, if -Wextra complains, then be it. I did have -Wextra among the compiler flags originally, until I realized that older gccs did not have the flag, and gave an error as a result. So, I removed it. Maybe, I can reinstate it for the CI build though...

[attipaci]

I wonder why the CI checks do not trigger for this PR...

OK, found it. I simply forgot to add pull_request to the triggers. For my own PRs the push trigger was sufficient, but not for forked ones... I pushed the workflow changes to main. You should rebase this PR, and force push, and hopefully the CI will then run properly....

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.64%. Comparing base (c6ef9ef) to head (81d7c11).
Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main      #66   +/-   ##
=======================================
  Coverage   99.64%   99.64%           
=======================================
  Files           7        7           
  Lines        2795     2802    +7     
  Branches      540      544    +4     
=======================================
+ Hits         2785     2792    +7     
  Partials       10       10           

Flag	Coverage Δ
unittests	99.64% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
src/frames.c	99.52% <ø> (ø)
src/novas.c	99.54% <100.00%> (-0.01%)	⬇️
src/refract.c	100.00% <ø> (ø)
src/super.c	100.00% <ø> (ø)

... and 1 file with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6ef9ef...81d7c11. Read the comment docs.

[kiranshila]

I think this should be good to go. I'll let you chew on what to do with that last unresolved comment - but this builds with -Wall -Wextra -pedantic -std=c99 with no warnings.

[attipaci]

I'm going to squash merge it, since the total number of changes are relatively few, and spread over many commits, with some back and forth... I hope you don't mind...

[kiranshila]

I'm going to squash merge it, since the total number of changes are relatively few, and spread over many commits, with some back and forth... I hope you don't mind...

Of course! I intended to squash if you didn't :)

[attipaci]

And, once again @kiranshila , thanks for the good work, and the time and effort you put into it. :-)