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

Refactor Lexer to support an abstract InputSource class #1363

Merged
merged 1 commit into from
Jul 7, 2022
Merged

Conversation

philberty
Copy link
Member

@philberty philberty commented Jul 6, 2022

This patch allows us to remove the fmemopen lex_string hack to support
parsing buffers. This will allow us to support mutliple sources such as
metadata imports etc. The patch here updates the parser to hold onto a
reference to the lexer rather than 'owning' the lexer which allows us to
decouple the move semantics here.

Fixes #1203
Fixes #1000

@philberty philberty added the bug label Jul 6, 2022
@philberty philberty added this to the Imports and visibility milestone Jul 6, 2022
@philberty philberty added this to In progress in Imports and Visbility via automation Jul 6, 2022
@philberty philberty assigned philberty and unassigned philberty Jul 6, 2022
@philberty philberty requested a review from dafaust July 6, 2022 20:42
Copy link
Member

@CohenArthur CohenArthur left a comment

Choose a reason for hiding this comment

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

Nice changes. I like that

gcc/rust/lex/rust-lex.h Outdated Show resolved Hide resolved
gcc/rust/lex/rust-lex.cc Show resolved Hide resolved
gcc/rust/lex/rust-lex.cc Show resolved Hide resolved
gcc/rust/lex/rust-lex.cc Outdated Show resolved Hide resolved
@SimplyTheOther
Copy link
Member

Nice job.

My only reservation with this (apart from CohenArthur's raw pointer bit above) would be the potential performance hit of indirection for the input source. Since there's now a virtual call to get every new character in the lexer, the extra vtable lookups as a result could actually have a noticeable effect on overall performance. A template-based approach as mentioned in #1000 would avoid this performance hit at the cost of a somewhat more complicated program, slower compilation time, possibly larger generated binaries, and worse error messages on compile.

I could be wrong about how much performance loss actually occurs if something else during lexing and parsing (e.g. I/O) has a much greater effect than the virtual calls, however.

This patch allows us to remove the fmemopen lex_string hack to support
parsing buffers. This will allow us to support mutliple sources such as
metadata imports etc. The patch here updates the parser to hold onto a
reference to the lexer rather than 'owning' the lexer which allows us to
decouple the move semantics here.

Fixes #1203 #1000
@philberty
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Jul 7, 2022

Build succeeded:

@bors bors bot merged commit d4dda78 into master Jul 7, 2022
Imports and Visbility automation moved this from In progress to Done Jul 7, 2022
@philberty philberty deleted the phil/lexer-fix branch July 7, 2022 14:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

error: 'fmemopen' was not declared in this scope Fix unsafe lex_string() implementation
3 participants