'static' keyword gets added to a function forward declaration based on its body contents

[gtrainavicius]

arduino-builder/ctags/ctags_parser.go

Line 105 in 29cf4c8

if strings.Index(tag.Code, STATIC+" ") != -1 {

Issue Description

I hit this issue after writing a complex macro which defines a 'strong' function implementation, and the code generated by the macro must be written in a very peculiar way. The idea is to allow easy overriding of a default 'weak' function, in order to let user easily customize an Arduino library I am writing without having to rely on config headers or custom board specifications. (https://github.com/BlokasLabs/USBMIDI, will commit the actual macros soon, will add a comment with a link to them)

The issue is that after preprocessing, the entire function lands in a single line, and the 'static' keyword gets added because such keyword is found within the implementation, but the Arduino Builder should only be concerned about the function prototype itself.

Example

Here's an example macro that would reproduce it if placed in .ino:

#define TEST() int test() \
    { \
        static int x = 5; \
        return x; \
    }

TEST();

The resulting forward declaration in the preprocessed sketch would end up like:

static int test();

int test() { static int x = 5; return x; };

Since the forward declaration is marked static, other compiled object files will not be able to see this implementation, and therefore the default implementation marked with the 'weak' attribute does not get overridden.

Expectation

The expected forward declaration would be int test();

Solution

I know that the current solution to workaround this would be to put this macro in a .cpp file, however, I would love to achieve the simplicity of simply dropping a macro call in single .ino file :)

I'd like to suggest making the check at:

arduino-builder/ctags/ctags_parser.go

Line 105 in 29cf4c8

if strings.Index(tag.Code, STATIC+" ") != -1 {

to check whether the 'static' that was found is not after a '(' or '{' within the same line, that should let it discard 'static' that could appear in the implementation body.

Additionally, there should be a check whether the 'static' is separated by whitespace from both sides, a bonus test case:

int function_static () // Note the space before ()
{
 return 5;
} 

Results in:

static int function_static();

[gtrainavicius]

And one more test case:

void test() // static used in a comment on the same line
{
}

gets forward declared as:

static void test();

[facchinm]

Wow, thanks for the very precise report.
Of course adding the static keyword in both cases is a bug; would you mind testing the same code by passing -experimental to the builder? This will trigger arduino-preprocessor to forward declare the prototypes instead than unhealthy ctags + manual parsing. For the sake of simplicity the beta version of the IDE already invokes it with that flag, if you prefer to test in a "real world" scenario.

[gtrainavicius]

As promised, here's the macros: https://github.com/BlokasLabs/USBMIDI/blob/master/src/usbmidi.h#L54

And here's the default instantiation using the weak attribute: https://github.com/BlokasLabs/USBMIDI/blob/master/src/usbmidi_vusb.cpp#L199

As it's pretty difficult for Library developers to provide 'compile time' customization via preprocessor defines in the compiler command line, I'd definitely recommend use of weak symbols for Arduino. :)

Ok, I have tested with 1.9.0-beta and the issue does not reproduce - the macros work fine if placed in .ino file, so I guess this issue can be closed.

Btw, when do you expect to publicly release 1.9.0?

[facchinm]

There are still a few regressions to be targeted before we can safely merge arduino-preprocessor support; of course testing is always welcome, so if you could use the build for a few weeks and report any regression you may find it would be great!