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

Fix string encodings. #62

Merged
merged 6 commits into from
Nov 16, 2020
Merged

Fix string encodings. #62

merged 6 commits into from
Nov 16, 2020

Conversation

bgamari
Copy link
Contributor

@bgamari bgamari commented May 17, 2020

Previously we used the Binary instance for Text to serialise the event
name. This is wrong.

We now first encode to UTF-8 and use this in the eventlog encoding.

@bgamari bgamari mentioned this pull request May 17, 2020
7 tasks
@bgamari
Copy link
Contributor Author

bgamari commented May 17, 2020

Unfortunately testing this is quite non-trivial due to the fact that EVENT_STEAL_SPARK is translated to SparkSteal and therefore can't roundtrip.

@maoe
Copy link
Member

maoe commented May 18, 2020

Whoops. Apparently I wasn't careful enough when I was writing #55. The changes look good to me.

The problem is that the new test is failing due to #14. Could you disable the test until #14 is fixed?

test/Roundtrip.hs Outdated Show resolved Hide resolved
src/GHC/RTS/Events/Binary.hs Outdated Show resolved Hide resolved
ghc-events.cabal Show resolved Hide resolved
@maoe
Copy link
Member

maoe commented Nov 16, 2020

I'll take over this PR.

bgamari and others added 6 commits November 17, 2020 06:30
Previously we used the Binary instance for Text to serialise the event
name. This is wrong.

We now first encode to UTF-8 and use this in the eventlog encoding.
This test fails due to haskell#14. Until it's fixed we disable the test.
@maoe
Copy link
Member

maoe commented Nov 16, 2020

Confirmed that all tests (excluding the disabled ones) passed in #65. Merging.

@maoe maoe merged commit 216dc55 into haskell:master Nov 16, 2020
@bgamari
Copy link
Contributor Author

bgamari commented Nov 18, 2020

Thanks @maoe!

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.

2 participants