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

Allow whitespace control chars in EventId texts. #208

Merged
merged 1 commit into from
May 30, 2023

Conversation

andjo403
Copy link
Contributor

As it only seems that control chars is not allowed in the EventId texts is to allow for future tags I think excluding the whitespace control chars is ok.
What I can see right now the only tag used is for arguments and is '\x1E'.

fixes #207

cc @michaelwoerister

@michaelwoerister michaelwoerister self-assigned this May 26, 2023
@michaelwoerister
Copy link
Member

Nice! Thanks, @andjo403 🙂

I'll need to take a closer look at the entire file format, to make sure this doesn't have any unintended consequences. It's been a while since I worked with this code.

But I believe the suggested change matches the originally intended behavior.

@andjo403
Copy link
Contributor Author

did a test with assigning all the whitespace control chars to the label and args and crox seems to handle it correct at least.
let event_label = profiler.alloc_string("l\ta\n\rbe\x0Cl"); let event_arg = profiler.alloc_string("a\tr\n\rg\x0Cs");

image

but good that you look at the entire file format.

@michaelwoerister
Copy link
Member

OK, this looks good! Thanks again, @andjo403!

@michaelwoerister michaelwoerister merged commit e136131 into rust-lang:master May 30, 2023
@andjo403 andjo403 deleted the allowWsInEventTexts branch May 30, 2023 18:30
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.

Could not parse event_id. Found ASCII control character in <text>
2 participants