Fix handling of macro with empty argument list

[ojwb]

SWIG's preprocessor doesn't work quite like the preprocessors in C/C++ compilers (I compared it to GCC and clang) on this example:

#define FOO(X) int foo(X);
#define BAR() int bar();

const int FOO = 7;
const int BAR = 6;
FOO(int x)
BAR()
FOO()
// This should be an error
// BAR(ignored)

Without this patch, SWIG gives:

const int FOO = 7;
const int int bar(); = 6;
int foo(int x);
int bar();
FOO

while GCC and clang give:

const int FOO = 7;
const int BAR = 6;
int foo(int x);
int bar();
int foo();

Work-in-progress patch currently - I wanted to check it didn't break anything, but locally the testsuite fails due to me having a newer pep8, and I also don't have all the languages installed.

[ojwb]

CI tests are all green, so it looks like fixing this isn't going to break the world. There's clearly potential for breaking existing user interface files which rely on the current behaviour, but I think we just have to accept that risk (and for 4.0 it seems reasonable) - SWIG really ought to behave like a C/C++ compiler's preprocessor in cases like this (I hit this in a real world example, and I'm struggling to see how to work around it).

Please don't merge yet - I want to rework the approach a bit regarding how the empty argument list is represented, and it needs a CHANGES.current entry.

[wsfulton]

This will be a nice addition. Note that we work around this in places. Will it fix this the closely related empty argument bug too, eg in Lib/shared_ptr.i:

// Workaround empty first macro argument bug
#define SWIGEMPTYHACK
// Main user macro for defining shared_ptr typemaps for both const and non-const pointer types
%define %shared_ptr(TYPE...)
%feature("smartptr", noblock=1) TYPE { SWIG_SHARED_PTR_QNAMESPACE::shared_ptr< TYPE > }
SWIG_SHARED_PTR_TYPEMAPS(SWIGEMPTYHACK, TYPE)
SWIG_SHARED_PTR_TYPEMAPS(const, TYPE)
%enddef

[ojwb]

It didn't fix that case, but it's related to the bug here and I've pushed an adjustment to the fix which I think should cover this too. Thanks for pointing this out.

[ojwb]

Note that we work around this in places

Do you know where we work around the original bug here, or how to locate such places?

[wsfulton]

Mmm, probably the only workaround is the first argument bug and not the original no arguments bug in this issue. I have seen the no arguments bug brought up before, but not sure where, possibly in the SF bug tracker. Sorry, that isn't particularly helpful!

[ojwb]

The no arguments bug did seem very vaguely familiar, but I didn't see an open issue here. It seems the sourceforge ticket search is broken currently, and I didn't find anything via google with a suitable site: filter, but google probably don't have complete coverage.

[ojwb]

I found https://sourceforge.net/p/swig/bugs/826/ which is about a varargs macro not accepting an empty argument list, and which this change also fixed, so I've added that to the regression test and closed it.

[wsfulton]

In the preproc.i testcase:

#define FOO2(X) int foo_func2(X);
...
FOO2()

gcc, clang and visual c++ preprocessor output is:

int foo_func2();

gcc and clang are silent and Visual c++ emits a warning:

preproc_wrap.c(3345): warning C4003: not enough actual parameters for macro 'FOO2'

SWIG outputs:

FOO2()

So I think this is another similar case where SWIG ought to follow the compilers, possibly with an 'extra' warning which is suppressed by default unless using -Wextra or -Wall.

[ojwb]

I'm inclined to think "C4003" is just another pointless MSVC warning and not something we should emulate (unless the C or C++ standard says it's not valid, which I don't have time to check right now).

But the testcase isn't working properly and there are some actual bugs:

SWIG git master:

// This has probably always worked, but make sure that the fix to accept
// an empty X doesn't cause this case to be incorrectly expanded:
const int FOO = 7;
// BAR was incorrectly expanded here, causing:
// Error: Syntax error in input(1).
const int BAR = 6;
// This has probably always worked, but make sure that the fix to accept
// an empty X doesn't stop a non-empty X from working:
FOO(int x)
// FOO() didn't used to get expanded here, causing:
// Syntax error in input(1).
FOO2()
// Check BAR2() still gets expanded here.
BAR2() {
    // Regression test - this used to fail with:
    // Error: Macro 'BAZ' expects 3 arguments
    BAZ(,2,3);
    BARVAR();
    FOOVAR(1);
    BAZVAR(1,2,3);
    return 0;
}

GCC and clang:

const int FOO = 7;


const int BAR = 6;


int foo_func(int x);


int foo_func2();

int bar_func2() {


    baz_func(+0,2,3);
    bar_func();
    foo_func(1);
    baz_func(1,2,3);
    return 0;
}

I guess this is a C testcase and so SWIG is assume int as the return type of FOO, FOO2 and BAR, which is why it's not failing (and why I missed this).

Thanks for spotting - I'll work on a fix, though it may be a few days before I have time.

Meanwhile I think we can leave the current patch on git master - SWIG 3.0.12 gives this, and git master is either the same or better:

// This has probably always worked, but make sure that the fix to accept
// an empty X doesn't cause this case to be incorrectly expanded:
const int FOO = 7;
// BAR was incorrectly expanded here, causing:
// Error: Syntax error in input(1).
const int BAR = 6;
// This has probably always worked, but make sure that the fix to accept
// an empty X doesn't stop a non-empty X from working:
FOO(int x)
// FOO() didn't used to get expanded here, causing:
// Syntax error in input(1).
FOO2()
// Check BAR2() still gets expanded here.
BAR2() {
    // Regression test - this used to fail with:
    // Error: Macro 'BAZ' expects 3 arguments
    BAZ(,2,3);
    BARVAR();
    FOOVAR(1);
    BAZVAR(1,2,3);
    return 0;
}

[wsfulton]

SWIG aims to support ANSI/ISO C and I don't recall SWIG attempting to support K&R C where the return type defaults to int if left off.

ff(int a);

results in a syntax error in SWIG.

Agreed the patch should be left as it is provides much better preproc support.

[ojwb]

I thought implicit int return and parameter type was valid C90 and only removed in C99, and GCC seems to agree:

$ echo 'foo(x) { return x; }' | gcc -std=c90 -pedantic -xc - -c -o /dev/null
$ echo 'foo(x) { return x; }' | gcc -std=c99 -pedantic -xc - -c -o /dev/null
<stdin>:1:1: warning: return type defaults to ‘int’ [-Wimplicit-int]
<stdin>: In function ‘foo’:
<stdin>:1:1: warning: type of ‘x’ defaults to ‘int’ [-Wimplicit-int]

But as you say it SWIG doesn't seem to support it (and nobody's complained that I can recall).

I wonder why FOO(int x) in my testcase doesn't cause SWIG to fail with a parse error then...

[ojwb]

The output I quoted above for SWIG is bogus - it's what you get from swig -E preproc.i but because it's inside a %inline block, it's actually just a verbatim copy of the input, not what SWIG's preprocessor produces.

If I use just the content of that %inline block, i.e.:

#define FOO(X) int foo_func(X);
#define FOO2(X) int foo_func2(X);
#define BAR() int bar_func();
#define BAR2() int bar_func2()
#define BAZ(A,B,C) baz_func(A+0,B,C)
#define FOOVAR(...) foo_func(__VA_ARGS__)
#define BARVAR(...) bar_func(__VA_ARGS__)
#define BAZVAR(...) baz_func(__VA_ARGS__)
// This has probably always worked, but make sure that the fix to accept
// an empty X doesn't cause this case to be incorrectly expanded:
const int FOO = 7;
// BAR was incorrectly expanded here, causing:
// Error: Syntax error in input(1).
const int BAR = 6;
// This has probably always worked, but make sure that the fix to accept
// an empty X doesn't stop a non-empty X from working:
FOO(int x)
// FOO() didn't used to get expanded here, causing:
// Syntax error in input(1).
FOO2()
// Check BAR2() still gets expanded here.
BAR2() {
    // Regression test - this used to fail with:
    // Error: Macro 'BAZ' expects 3 arguments
    BAZ(,2,3);
    BARVAR();
    FOOVAR(1);
    BAZVAR(1,2,3);
    return 0;
}

SWIG 3.0.12 fails with:

gh1111.i:25: Error: Macro 'BAZ' expects 3 arguments

Which is good, as that means that part is working as a regression test.

SWIG git master gives:

// This has probably always worked, but make sure that the fix to accept
// an empty X doesn't cause this case to be incorrectly expanded:
const int FOO = 7;
// BAR was incorrectly expanded here, causing:
// Error: Syntax error in input(1).
const int BAR = 6;
// This has probably always worked, but make sure that the fix to accept
// an empty X doesn't stop a non-empty X from working:
int foo_func(int x);
// FOO() didn't used to get expanded here, causing:
// Syntax error in input(1).
int foo_func2();
// Check BAR2() still gets expanded here.
int bar_func2() {
    // Regression test - this used to fail with:
    // Error: Macro 'BAZ' expects 3 arguments
    baz_func(+0,2,3);
    bar_func();
    foo_func(1);
    baz_func(1,2,3);
    return 0;
}

Which matches GCC and clang preprocessed output (if you run them with -C -E then it's exactly identical, except for all the boilerplate stuff at the start of the file).

So it actually looks like the FOO2() case is already handled correctly. I'm guessing you were misled by the verbatim output from the %inline block like I was?

[wsfulton]

Oh good, it all works as it should. Yes, I looked at the preprocessed output and fell into the same trap. A runtime test calling these functions would be a useful enhancement to make sure that it all expands as expected.

[ojwb]

On 13 October 2017 at 19:10, William S Fulton ***@***.***> wrote: A runtime test calling these functions would be a useful enhancement to make sure that it all expands as expected.

I actually constructed the testcases such that SWIG should fail at the preprocessor or parser stage if any of the fixes regresses. Cheers, Olly