Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement yyset_interactive #291

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rmccullagh
Copy link
Contributor

@rmccullagh rmccullagh commented Dec 12, 2017

Currently, only available in reentrant mode, since it needs a scanner argument. However, I believe a yyinteractive macro can be implemented too. References #290

@rmccullagh rmccullagh changed the title implement yyset_interactive [#290] implement yyset_interactive Dec 12, 2017
src/flex.skl Outdated
@@ -618,6 +619,7 @@ void yyfree ( void * M4_YY_PROTO_LAST_ARG );

m4_ifdef( [[M4_YY_NOT_IN_HEADER]],
[[

#define yy_new_buffer yy_create_buffer
#define yy_set_interactive(is_interactive) \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you are implementing yyset_interactive, could you also remove this yy_set_interactive macro, which could cause confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Explorer09 Good call, I will get this updated tonight.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Explorer09 This is done.

@jannick0
Copy link
Contributor

jannick0 commented Dec 12, 2017

Sorry for my ignorance: Why not simply amending the definition of the macro yy_set_interactive using the m4 variables M4_YY_PROTO_LAST_ARG and M4_YY_DECL_GUTS_VAR already defined in flex.skl?

[EDIT] ... like so?

--- C:/myDevel/myprogs/flex/flex_2.6.4/yy_interactive/flex1.skl	Wed Aug 23 13:00:27 2017
+++ C:/myDevel/myprogs/flex/flex_2.6.4/yy_interactive/flex.skl	Tue Dec 12 17:19:38 2017
@@ -611,8 +611,9 @@
 m4_ifdef( [[M4_YY_NOT_IN_HEADER]],
 [[
 #define yy_new_buffer yy_create_buffer
-#define yy_set_interactive(is_interactive) \
+#define yy_set_interactive(is_interactive M4_YY_CALL_LAST_ARG) \
 	{ \
+    M4_YY_DECL_GUTS_VAR(); \
 	if ( ! YY_CURRENT_BUFFER ){ \
         yyensure_buffer_stack (M4_YY_CALL_ONLY_ARG); \
 		YY_CURRENT_BUFFER_LVALUE =    \

Are there any issues when doing so? Of course, I see the advantage of having a function instead of a macro, especially from another c file, however, macros are preferred by flex.

Separately, would it be fair to say that the remainder of the changes are enhancements to command line options only?

@Explorer09
Copy link
Contributor

@jannick0 The use of the function instead of a macro is because YY_CURRENT_BUFFER_LVALUE is an internal macro, AFAIK. That won't be the problem if yy_set_interactive is called only within the .l file, but that's not the case.

@jannick0
Copy link
Contributor

@Explorer09 Agree, but the issue then becomes a philosophical one, namely if the implementation should prefer macros (as is essentially right now) or functions. Using a wrapper would a way out of the current situation to effectively call macro defined in the lex file only without any changes.

The short patch above is amending what I would consider a bug, since for reentrant scanners the opaque scanner object needs to be resolved as it contains the flag is_interactive as a member to be set.

@rmccullagh
Copy link
Contributor Author

@jannick0 If you read #290, you can see that yy_set_interactive is perfectly callable from the .l file, but if you need more control, you don't have access to that macro because it is not defined in a header file. Furthermore, the required data structure to implement yy_set_interactive is an internal data structure.

@Explorer09
Copy link
Contributor

There are weaknesses of C macro mainly because they work like simple string substitution. If you only use C macro for reasons of function inlining, please don't, or do it sparingly.

I should have mentioned that yy_set_interactive, in it's current macro form, lack the proper do {...} while (0) enclosure. See this (https://stackoverflow.com/questions/257418/do-while-0-what-is-it-good-for) for reasons.
Above is one of the reasons you should avoid macro as much as possible. Modern compilers are excellent at inlining functions, so they can inline static functions when that benefits. Speaking of static functions, I would like to bring issue #277 together with this, since I would like if there's a way to make yyset_interactive static when we have the function implemented.

@jannick0
Copy link
Contributor

Again, using macros or functions is a question of coding style which is not my business here, supposedly rather the package maintainer's. Other issues about how to use macros are interesting, but to be dealt with in a different place I would say. Of course, I agree that they should certainly be followed for implemented macros.

If a function implementation of yy_set_interactive is ultimately chosen, then I would strongly recommend not defining it as static sucht that it can be made available in other entities. [BTW to avoid any confusion: Even the macro implementation of any procedure like yy_set_interactive (not defined in the flex header file) can be made available in other entities using a public function wrapper defined in the .l file as long as the opaque scanner object type is available in that very entity. The latter is needed either way, the macro of the function implementation.]

@westes westes added this to the 2.6.6 milestone Dec 15, 2017
@westes
Copy link
Owner

westes commented Apr 25, 2024

Could you rebase and resolve conflicts? As you can see, we're (finally) clearing out the backlog.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants