Use swig -Werror for test suite

[ojwb]

I'm really not sure what's going on with the scilab warnings:

.././../li_std_string_extra.i:12: Warning 402: Base class 'std::string' is incomplete.
../../../../Lib/typemaps/std_string.swg:16: Warning 402: Only forward declaration 'std::string' was found.

Makes me wonder if there's something not right in the SWIG/Scilab's std::string library support, but I'm not spotting what's different that's causing this compared to other languages.

[ojwb]

Most target languages are already warning-clean and for the minority that aren't this suppresses the current warnings we get from the testsuite, and then enforces by adding -Wall. I didn't bother with mzscheme as that has a lot of warnings and is slated to be removed in 4.4.0, but did all the others.

Some of these warnings inevitably indicate actual bugs (probably target language-specific ones), but I don't think it makes sense to block merging this on investigating and addressing them - those bugs are already present rather than something this branch introduces, and by merging this we at least prevent further warnings creeping in but being missed because there are already lots of warnings for that target language.

For example, the Go const static getters issue (fixed in d8d6b733ed621fd0949b5ad94444562404c8d3bf) was something I only spotted because I worked on this, and would never have crept in in the first place if we'd already had -Wall enforcing no new testsuite warnings.

(I even reviewed partialcheck output for the change which introduced that, but missed this change - not helped by SWIG/Go using a hash of the input file contents in generated identifiers, so they all change as soon as you modify a testcase.)

I don't see a particular benefit to merging this before 4.3.0 (having got the branch to this stage was useful) but I think we should merge once the release is out.

[wsfulton]

I'm not sure I agree with all these warning suppressions. A lot of these warnings need addressing instead of sweeping under the carpet. Particularly the mass of Go warnings that were suppressed. I've pretty much left warnings that need addressing on. If we are to have the test-suite warning free, and I think this is a fairly high priority in order to avoid further warnings appearing, these problems should be addressed first. I'd rather see some sort of plan to address the warnings and turn them back on until addressed. I am happy to help. Maybe we could tackle a language each or something??

[ojwb]

My change doesn't "sweep them under the carpet" - if they're now under a carpet, it's just a different patterned carpet to before. Before they swept by in testsuite output; now they are effectively listed in the testcase code.

The key problem with having our todo list for these flash by in testsuite output is that it's easy to miss when a new warning is introduced, particularly for target languages which already have a lot of warnings (e.g. I only spotted the bug fixed by d8d6b733ed621fd0949b5ad94444562404c8d3bf because I was working on this patch).

I'd rather see some sort of plan to address the warnings and turn them back on until addressed. I am happy to help. Maybe we could tackle a language each or something??

Turning them back on just isn't a sensible option - it'll inevitably mean we miss bugs that would have been automatically caught by CI.

Sure these warnings ought to be addressed but that was equally true before my change but nobody was actually addressing them.

I'd love to fix them but I have no idea what the problem is for most of them - e.g. I looked at the scilab std::string ones as I mentioned above but couldn't work out what was wrong.

Particularly the mass of Go warnings that were suppressed.

The Go target language seems to be steadily accruing bug reports (24 currently). I think SWIG/Go really needs more active maintenance in general, but it's not a language I use or really know a lot about.

Would more clearly marking the warning suppressions which are bugs make you happier? (Rather than those needed due to features of the target language - e.g. only having one floating point type.)

[ojwb]

Looking through:

• Lua's OK - there is an underlying issue that SWIG doesn't support Lua's new-ish integer type but we have #1918 open for that
• Guile seems to be down to four things:
  • size_t in cpp11_brackets_expression.i is not known to SWIG - I'll adjust the testcase to use unsigned instead and drop these suppressions (size_t isn't important for what it is testing)
  • SWIG/Guile isn't able to wrap function pointers as constants, which seems a reasonable limitation
  • SWIG/Guile isn't able to wrap std::string which seems due to a hard-coded check in guile.cxx that returns SWIG_NOWRAP if ((SwigType_type(nctype) == T_USER) && (!is_a_pointer(nctype))). That seems the wrong check - it ought to check if it has a suitable typemap like other languages seem to (except mzscheme which has similar code to guile). There's a check just after for a constant typemap (although there seem to be no such typemaps defined anywhere!) otherwise it seems to wrap via variableWrapper() so a varout typemap is what's needed. However we can't call Swig_typemap_lookup()in constantWrapper() to check if there is one since then the second call to do that in variableWrapper() asserts, and there doesn't seem a good way to get variableWrapper() to check for us without a major refactor which I'm not confident to do myself.
  • Guile only seems to have one integer and one floating point type
  • R's OK - it just only seems to have one integer type
  • Scilab - std::string is apparently incomplete?!
  • Go - quite a lot of issues

Would more clearly marking the warning suppressions which are bugs make you happier? (Rather than those needed due to features of the target language - e.g. only having one floating point type.)

Thinking about this more, I'd suggest we instead open tickets for the cases where you think the suppressions are hiding actual bugs. It seems clear some of the Go ones are, probably the Guile constant object wrapping limitation is, maybe the Scilab std::string incomplete warning too; nothing else seems to be. An open ticket seems a much better way to track a bug than either a warning flashing by in testsuite output or a comment annotating a warning suppression in a testcase.

[ojwb]

there doesn't seem a good way to get variableWrapper() to check for us without a major refactor which I'm not confident to do myself.

A cunning plan occurred to me - we can just set a flag with a special name on the fake variable node that constantWrapper() creates and check for that in variableWrapper() - if it is set we don't warn and just return SWIG_ERROR which constantWrapper() can check for.

I've opened #3056 with changes to do that. That deals with the warnings for the std::string constants and two of the function pointer constants so Guile looks good now.

[ojwb]

Thinking about this more, I'd suggest we instead open tickets for the cases where you think the suppressions are hiding actual bugs. It seems clear some of the Go ones are, probably the Guile constant object wrapping limitation is, maybe the Scilab std::string incomplete warning too; nothing else seems to be. An open ticket seems a much better way to track a bug than either a warning flashing by in testsuite output or a comment annotating a warning suppression in a testcase.

Guile now fixed; R and Lua are OK; opened new issues #3150 for Scilab and #3151 for Go.