-
Notifications
You must be signed in to change notification settings - Fork 245
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
Sanity checking FLINT #2085
base: main
Are you sure you want to change the base?
Sanity checking FLINT #2085
Conversation
Adds a lot of compiler warnings, and turns warnings into errors. Maintainer-level only.
Removes -Wsign-compare, -Wmissing-prototypes and -Wmissing-declarations.
I'm not so happy sprinkling the code with
instead of just providing the missing prototypes. |
I'm not too happy with providing prototypes in files, I would rather have |
I don't understand the need for |
There was one instance where a global function was needed, hence the definition. |
Which one? It should be possible to change that. |
Hmm, it was more than I remember. There is stuff like _ARB_DEF_CACHED_CONSTANT(static, static, _acb_hypgeom_const_li2, _acb_hypgeom_const_li2_eval) for local functions, and then stuff like ARB_DEF_CACHED_CONSTANT(arb_const_khinchin, arb_const_khinchin_eval) global functions. There is quite a lot of both. |
I've tried to write this up in a nice way, but I'm sure I overlooked some things or explained it in a poor way. Please don't hesitate to ask questions, disagree or give feedback.
Background
FLINT is quite a big repository, and so, in my opinion, it is necessary to have some sort of guards against a "diverging" repository, and a coding convention that not only makes it easier to understand the repository but also tracks the dependencies within files, between files and between modules.
Examples in the source code that acts against this are in my opinion:
x
, and you now choose to represent it as a floating-point, keeping the variable name to signal that it represents the same value), but I think it is better to avoid this.slong
andulong
), what is actually meant? Do you castulong
toslong
? The other way around? Or do you intend to not do any sort of cast, and do the mathematical comparison?XXX-impl.h
) or a global header (such asfmpz.h
). Otherwise it becomes very hard to track dependencies, local versus global function etc.What this PR intends to do
Add an option
--enable-sanity-check
which compiles FLINT with a lot of, in my opinion, reasonable warnings. Also implement a CI runner that compiles all source code and tests.These warnings include:
-Wall
-- good collection of warnings, already enabled by default in FLINT,-Wextra
but without-Wcast-function-type
(incompatible with GR) -- enables extra warnings, including:-Wsign-compare
-- warns for sign-unsigned comparisons,-Wunused-parameter
-- warns for unused parameters,-Werror
-- turns warnings into errors in order to fail compilation,-Wshadow
-- warns for shadowed variables,-Wredundant-decls
-- warns for redundant declarations,-Wmissing-prototypes
and-Wmissing-declarations
-- warns for global functions that does not have any prior prototypes/declarations.What has changed
Functions that looks to be intended as global functions, but are never required to compile FLINT, push
# pragma message "MYFUNC is currently unused/untested/undocumented!"
to the compiler. This is a manual and temporary "fix", and it is only really intended to highlight these functions for maintainers and contributors.
For functions that should be local but are required for tests, push
# pragma message "MYFUNC only needs a symbol for test"
Is also a manual and temporary "fix".
Functions that can be declared as
static
, declare them asstatic
.Fix signed-unsigned comparisons by appropriate casting.
Remove redundant declarations.
Remove all
FLINT_UNUSED
in headers, and only use it in*.c
files to signal that certain parameters are unused.Introduce
FLINT_HEADER_START
andFLINT_HEADER_END
to guard against warnings for unused parameters in inline functions in header files (to avoid extensive use ofFLINT_UNUSED
).Commented out some unused functions that didn't look necessary via
#if 0 ... #endif
. The complete list can be see here:COMMENTED OUT FUNCTIONS
Please let me know if any of these functions needs to be preserved!
However, I still believe it would be a good idea to create
*-impl.h
headers to keep track of private/helper functions.TLDR
Make FLINT compile with a lot of warning options and incorporate this into the CI.
NOTE: Will squash the trailing commits before any merging.
EDIT: Added list of commented out functions.