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

Move some macro for universal parser #8044

Conversation

S-H-GAMELINKS
Copy link
Contributor

Move some macro that defined in internal/ctype.h to parse.y .

@S-H-GAMELINKS S-H-GAMELINKS force-pushed the remove/universal-parser-internal-ctype-depend branch 2 times, most recently from 01b93d6 to e17892f Compare July 8, 2023 15:11
parse.y Outdated
Comment on lines 89 to 92
#ifdef ISSPACE
#undef ISSPACE
#define ISSPACE parse_isspace
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Is #ifdef needed?
This means ISSPACE macro needs to be defined previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for comment.
ISSPACE macro already defined in include/ruby/internal/ctype.h and that included in ruby/ruby.h.
parse.y include ruby/ruby.h, and I thought #undef ISSPACE is needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment include/ruby/internal/ctype.h is included into parse.y in both cases of with UNIVERSAL_PARSER and without UNIVERSAL_PARSER. However we may remove such inclusion in the future if possible. Current code wrapped with #ifdef might not work once include/ruby/internal/ctype.h is not included because #define ISSPACE parse_isspace is not defined without ISSPACE definition ahead of this line.
Could you try to remove #ifdef ISSPACE ... #ifdef STRNCASECMP ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks comment and I understand. I'll try it.
And shoud I remove also #ifdef ISASCII ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed #ifdef ISSPACE ... #ifdef STRNCASECMP(also #ifdef ISASCII).
It' be fine that all test has passed, I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not only #ifdef ISSPACE and #ifdef ISASCII but also other #ifdef XXX like #ifdef ISCNTRL, #ifdef ISALPHA ...

By the way, current positions for parse_XXX is only for without UNIVERSAL_PARSER. Could you move these definitions to the place outside of UNIVERSAL_PARSER macro branches and RIPPER macro branches like

ruby/parse.y

Line 542 in 2903e94

?

# without UNIVERSAL_PARSER
$ nm parse.o | grep isspace
0000000000010ab0 t _parse_isspace
000000000002a9b8 t _rb_enc_isspace

# with UNIVERSAL_PARSER
$ nm parse.o | grep isspace
0000000000010a08 t _rb_isspace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks comment, It's fixed to move outside of UNIVERSAL_PARSER macro branches and RIPPER macro branche.

@S-H-GAMELINKS S-H-GAMELINKS force-pushed the remove/universal-parser-internal-ctype-depend branch 2 times, most recently from 30352c9 to a884201 Compare July 9, 2023 00:53
@S-H-GAMELINKS S-H-GAMELINKS force-pushed the remove/universal-parser-internal-ctype-depend branch from a884201 to 6870455 Compare July 9, 2023 04:26
@yui-knk yui-knk merged commit acd9c20 into ruby:master Jul 9, 2023
89 checks passed
@S-H-GAMELINKS S-H-GAMELINKS deleted the remove/universal-parser-internal-ctype-depend branch July 9, 2023 06:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants