-
Notifications
You must be signed in to change notification settings - Fork 13.4k
Support float-like tuple indices in offset_of!() #112216
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
Conversation
(rustbot has picked a reviewer for you, use r? to override) |
// Complex case: do all combinations of spacings because the spacing determines what gets | ||
// sent to the lexer. | ||
println!("{}", offset_of!(ComplexTup, 0.1.1.1)); | ||
println!("{}", builtin # offset_of(ComplexTup, 0. 1.1.1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this isn't supported by the offset_of!()
macro's declaration:
pub macro offset_of($Container:ty, $($fields:tt).+ $(,)?) {
builtin # offset_of($Container, $($fields).+)
}
Does it make sense to change it to:
pub macro offset_of($Container:ty, $($fields:tt)+) {
builtin # offset_of($Container, $($fields)+)
}
and then let the builtin syntax deal with it?
This pattern is supported for normal tuple field access.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm yeah that's a bit complicated to do because it means that the builtin offset_of
needs to get way better when it comes to errors... maybe it would be good for a follow up PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the macro
arguments weren't somewhat important as documentation, I'd rather go with
macro offset_of($($tt:tt)+) { ... }
and do all the work in the parser.
0793810
to
c7b72af
Compare
Purely a refactor in preparation of using it in offset_of!() parsing
c7b72af
to
07a719a
Compare
The tokenizer gives us whole float literal tokens, we have to split them up in order to be able to create field access from them.
07a719a
to
1b90f5e
Compare
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (43062c4): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 649.669s -> 649.217s (-0.07%) |
Supports invocations like
offset_of!((((), ()), ()), 0.0)
. This0.0
gets tokenized as float literal, so it has to be broken up again.The code that did the breaking up was returning a finished
Expr
, while we need aIdent
, so this PR splits up theparse_expr_tuple_field_access_float
function into:TokenKind::break_two_token_op
, but we do access the parser during this splitting operation, so we keep it as an inherent function on the parser)Expr
from itThe former we can then re-use in
offset_of
parsing. The edge cases especially involving whitespaces are tricky so this adds a bunch of new tests as well.fixes #112204