Sanity checking FLINT

[albinahlback]

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:

• Unused functions, that is, symbols in the library that are never used by anyone anywhere. My best guess is that these function are either a work in progress, forgotten to be used/documented/whatever, or that they where once a dependency for something else whose dependency was later removed.
• Shadowed variables (see Wikipedia). Makes it harder to follow code (also impossible to track variables with GDB?). I understand that it can be nice in some cases (say you have an integer 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.
• Signed-unsigned comparison. When you compare a signed integer with an unsigned integer (slong and ulong), what is actually meant? Do you cast ulong to slong? The other way around? Or do you intend to not do any sort of cast, and do the mathematical comparison?
• Redundant declarations.
• Missing prototypes and declarations before a global function is defined. A global function should be declared in a header, whether that is a private header (something like XXX-impl.h) or a global header (such as fmpz.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 as static.

• 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 and FLINT_HEADER_END to guard against warnings for unused parameters in inline functions in header files (to avoid extensive use of FLINT_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

  mag_rsqrt_re_quadrant1_lower
  crt_print
  acb_dft_inverse_cyc
  acb_dirichlet_gauss_sum_ui
  _acb_dirichlet_platt_isolate_local_hardy_z_zeros
  acb_dirichlet_platt_isolate_local_hardy_z_zeros
  _acb_dirichlet_theta_arb_series
  acb_mat_is_lagom
  _acb_get_rad_mag
  _arb_hypgeom_gamma_upper_fmpq_inf_choose_N_rel
  ca_evaluate_fmpz_mpoly_iter
  ca_gamma_inert
  fmpz_mpoly_set_linear2_three_term_si
  fmpz_mod_mpoly_univar_set
  fmpz_mod_polyu1n_intp_reduce_sm_poly
  fmpz_mod_polyu1n_intp_lift_sm_poly
  fmpz_mod_polyu1n_intp_crt_sm_poly
  fmpz_mod_mpolyu_print_pretty
  fmpz_mod_mpolyu_one
  fmpz_mod_mpolyu_repack_bits_inplace
  fmpz_mpoly_to_fmpz_poly
  fmpz_mpoly_from_fmpz_poly
  fmpz_tpoly_print
  fmpz_poly_factor_print_pretty
  fmpzi_norm_approx_d_2exp
  fq_nmod_mpoly_to_mpolyuu_perm_deflate
  fq_nmod_mpoly_from_mpolyuu_perm_inflate
  fq_zech_mpoly_evaluate_all_ui
  fq_zech_mpoly_repack_bits
  _n_fq_poly_rem_basecase_
  nmod_mpolyu_msub
  _nmod_mpolyn_get_coeff
  _nmod_mpolyun_get_coeff
  nmod_mpoly_to_mpolyun_perm_deflate_bivar
  nmod_mpoly_to_mpolyun_perm_deflate
  qsieve_display_relation
  qsieve_is_relation
  

  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.

[fredrik-johansson]

I'm not so happy sprinkling the code with

/* FIXME: Remove this guard against warnings. Best thing would probably be to
 * implement an *-impl.h to keep track of local functions. */
#ifdef __GNUC__
# pragma GCC diagnostic ignored "-Wmissing-prototypes"
#endif

instead of just providing the missing prototypes.

[albinahlback]

I'm not so happy sprinkling the code with

/* FIXME: Remove this guard against warnings. Best thing would probably be to
 * implement an *-impl.h to keep track of local functions. */
#ifdef __GNUC__
# pragma GCC diagnostic ignored "-Wmissing-prototypes"
#endif

instead of just providing the missing prototypes.

I'm not too happy with providing prototypes in files, I would rather have *-impl.h to be able to track these private functions. What do you think about this?

[fredrik-johansson]

I don't understand the need for _ARB_DEF_CACHED_CONSTANT. Why not just insert static unconditionally in ARB_DEF_CACHED_CONSTANT? I don't think we ever need to access these generated functions non-locally.

[albinahlback]

I don't understand the need for _ARB_DEF_CACHED_CONSTANT. Why not just insert static unconditionally in ARB_DEF_CACHED_CONSTANT? I don't think we ever need to access these generated functions non-locally.

There was one instance where a global function was needed, hence the definition.

[fredrik-johansson]

There was one instance where a global function was needed, hence the definition.

Which one? It should be possible to change that.

[albinahlback]

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.

[albinahlback]

I have removed all pragmas, and I have inserted *-impl.h for modules that needed it.

[albinahlback]

I also redefined ARB_DEF_CACHED_CONSTANT and friends. I believe I am happy with this.

[fredrik-johansson]

Seeing the -impl.h headers, I'm unconvinced.

Many of our modules are already limited in scope to implementing a very specific feature. It makes no sense to split them any further.

Some of the functions that have ended up in -impl.h headers seem to be there because the declarations were missing, not because they were intended to be private.

I would sooner split off smaller implementation modules from larger modules where this makes sense semantically.

We could maybe introduce some FLINT_INTERNAL or FLINT_PRIVATE prefix for function declarations if people feel strongly about that kind of distinction. (Personally, I think this will just add decision anxiety...)

---

Most of everything else in this PR is fine though.

Marking functions as static is good.

I buy the argument against shadowed variables.

Having a CI with --enable-sanity-check is OK.

I think generally our signed-unsigned comparisons are safe, but at least casts improve clarity. One issue with inserting casts to unsigned, especially in loop conditionals, is that the compiler might generate worse code; probably some of these casts actually ought to be to signed.

[albinahlback]

Seeing the -impl.h headers, I'm unconvinced.

...

Some of the functions that have ended up in -impl.h headers seem to be there because the declarations were missing, not because they were intended to be private.

I sort of agree. Would you be fine with pushing everything in XXX-impl.h to XXX.h, say at the bottom of the file?

We could maybe introduce some FLINT_INTERNAL or FLINT_PRIVATE prefix for function declarations if people feel strongly about that kind of distinction. (Personally, I think this will just add decision anxiety...)

I don't think this is necessary. To be clear, I just want to have some system to track dependencies and such. To have *-impl.h files is not necessary, and it does bloat the header/file space which is what I suppose you are implying.

I think generally our signed-unsigned comparisons are safe, but at least casts improve clarity.

I agree that they are mostly safe as most of it has been index variables.

One issue with inserting casts to unsigned, especially in loop conditionals, is that the compiler might generate worse code; probably some of these casts actually ought to be to signed.

Do you have any examples in mind? It shouldn't really matter, unless it is some compiler bug, no? At least on ARM and x86, it should just be the matter of choosing the correct instruction.

[fredrik-johansson]

Would you be fine with pushing everything in XXX-impl.h to XXX.h, say at the bottom of the file?

Yes, that should be fine!

[fredrik-johansson]

Do you have any examples in mind? It shouldn't really matter, unless it is some compiler bug, no? At least on ARM and x86, it should just be the matter of choosing the correct instruction.

http://kristerw.blogspot.com/2016/02/how-undefined-signed-overflow-enables.html