westes / flex

The Fast Lexical Analyzer - scanner generator for lexing in C and C++
Other
3.61k stars 538 forks source link

skeleton: yywrap() forward declaration doesn't allow static definition #277

Open Explorer09 opened 7 years ago

Explorer09 commented 7 years ago

This lack of feature brings a small hindrance in code optimization. I discovered this when I tried to redefine Flex's own scanner's yywrap() function to static:

diff --git a/src/scan.l b/src/scan.l
index 5ac864a..8866c33 100644
--- a/src/scan.l
+++ b/src/scan.l
@@ -1019,7 +1019,7 @@
 %%

-int yywrap(void)
+static int yywrap(void)
        {
        if ( --num_input_files > 0 )
                {

And it broke with this gcc error:

scan.l:1022:12: error: static declaration of ‘yywrap’ follows non-static declaration

It doesn't seem like flex's own yywrap() function is called from outside scan.l and so ideally should be declared static. But flex's skeleton and generator doesn't yet allow this static definition.

jannick0 commented 6 years ago

Would the suggestion imply that every yywrap function in any flex output - apart of the 2-stage-compilation approach of flex - has to be static, too? In this case this would most probably break existing code using flex, hence I would opt not to support this.

I understand the suggestion as an enhancement of the flex code for which the following could be considered:

Explorer09 commented 6 years ago

@jannick0 You misunderstood about the static thing. I didn't say it has to be static; I'm saying that it could be static, and it's better have a way to specify so. I don't think YY_SKIP_WRAP is the right macro for this, since it's a boolean definition, and the name won't stand what it's for if we adopt it for this use, anyway. The prefix isn't either. It's supposed be a namespace prefix, you know? My feature request isn't about namespace, but to allow a custom declaration of yywrap so I can specify static or inline or gcc's __attribute__or whatever I want.

jannick0 commented 6 years ago

@Explorer09 :

You misunderstood about the static thing. I didn't say it has to be static; I'm saying that it could be static, and it's better have a way to specify so.

I am still struggling to understand if this should apply to yywrap as part of either the flex package or flex output (c / c++) or even both. Regardless of having an option or not, the remainder of the flex code should be changed to cover the case when the option to define yywrap as static is on.

I don't think YY_SKIP_WRAP is the right macro for this, since it's a boolean definition, and the name won't stand what it's for if we adopt it for this use, anyway.

Names could be easily changed, but #define YY_SKIP_WRAP should exactly do what you want, namely allowing the user to overwrite the default function declaration of yywrap. Needless to say that then you add any attribute or whatever you want to the declaration, too.

The prefix isn't either. It's supposed be a namespace prefix, you know? My feature request isn't about namespace, but to allow a custom declaration of yywrap so I can specify static or inline or gcc's attributeor whatever I want.

A namespace does not exist in c as you know, but with the prefix feature you can change the leading yy to whatever you want in c code. I mentioned that in case you wanted link two flex outputs together which certainly would yield a linking error if the two yywrap function are unchanged and not static. The prefix feature would help here. If this was meant for the flex code itself, I would be hesitating to change the declaration to static, unless it would definitely not break any existing code relying on flex.

Explorer09 commented 6 years ago

@jannick0 My concern is not with the linking (so it's nothing to do with prefix approach), but with optimization. The C compiler can't optimize out a function if it knows it would be called from outside the file. Interprocedure optimization / LTO is not yet the norm in C compiling. Although I can force the LTO in my code to compile, declaring a function static can save me from forcing to use the -lto flag everywhere. Compiler can and will try to optimize static functions with standard optimization options.

jannick0 commented 6 years ago

OK. Then why not using some declaration decorations like this? Pre-compile with flex -o yywrap.c yywrap.l.

/* yywrap.l */
    static inline int yywrap(void) { return 1; }
    #define YY_SKIP_YYWRAP
%%
.
%%
int main (int argc, char** argv)
{
    return yylex();
}
Explorer09 commented 6 years ago

YY_SKIP_YYWRAP was an undocumented workaround to not emit a yywrap (function or macro) definition in the skeleton, when noyywrap option is set, IIRC. Your solution looks like a hack, which I don't think is great for addressing the problem.

jannick0 commented 6 years ago

But this is exactly the situation you want I thought: Providing a function yywrap different from flex'es default macro. So why not simply dropping the line #define YY_SKIP_WRAP, since the suggestion works here for me regardless of what YY_SKIP_WRAP is set to? To be prefectly honest in each flex output YY_SKIP_YYWRAP is documented to be provided such that the user can overwrite flex'es default definitions. If this is a hack or a workaround in code older than more than 15 years is difficult to say for me, given that there are generations of code created with flex since then. I'll leave it with others and you to solve your problem.

Explorer09 commented 6 years ago

To be more clear, I was expecting an approach that is as clean as YY_DECL, in which you can customize the declaration of yylex(), or an approach cleaner than that. Custom declaration of YY_DECL also mean you can pass additional object references that you can use from within the scanner.

jannick0 commented 6 years ago

OK - this is a slightly different problem flex' fathers have already foreseen by providing reentrant scanners.

Use this not very meaningful demonstration example of an inlined function yywrap you can elegantly feed with external information using the extra member of the scanner object. My compiler is not complaining about not being able to inline the function.

/* yywrap_r.l */
%option reentrant

%{  
#ifdef _MSC_VER
    #define forceinline __forceinline
#elif defined(__GNUC__)
    #define forceinline __attribute__((always_inline)) inline
#else
    #define forceinline inline
#endif

extern yyscan_t yyget_extra(yyscan_t yyscanner);

static forceinline int yywrap(yyscan_t lexer)
{
    void* extra = yyget_extra( lexer );
    return extra == NULL; /* returns always 1 in this example */
}
%}

%%
.       printf("<%s>\n", yytext);
%%

int 
main (int argc, char** argv)
{
    yyscan_t lexer;
    void* extra = NULL;
    yylex_init( &lexer );
    yyset_extra(extra, lexer);
    yylex( lexer );
    yylex_destroy( lexer );

    return 0;
}
jannick0 commented 6 years ago

@Explorer09 Any issue with the suggested solution above? Otherwise we could close this issue since the objective you mentioned in the issue subject is achieved I would then say.

jannick0 commented 6 years ago

@westes , @Explorer09 : I would suggest to close this issue since solved together with variations of the original issue and not addressed for a good time now. Any objections? Thanks.