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

TH Lift #392

Merged
merged 2 commits into from
Jun 15, 2021
Merged

TH Lift #392

merged 2 commits into from
Jun 15, 2021

Conversation

phadej
Copy link
Contributor

@phadej phadej commented May 22, 2021

WIP, have to add SBS instance still.

Builds on top of #390, because there is better "primtiive" to pack Addr#, we can do better then is currently done in
https://hackage.haskell.org/package/th-lift-instances-0.1.18/docs/src/Instances.TH.Lift.html#line-281

@phadej
Copy link
Contributor Author

phadej commented May 22, 2021

I think there is some problem with bytestring, which I fail to pinpoint. See how CI fails

    binary: FAIL
      tests/bytestring-th.hs:50:
      expected: "\NUL\129\STX\ETX\NUL\SOH\STX\ETX"
       but got: "\NUL\SOH\STX\ETX\NUL\SOH\STX\ETX"

yet the code

    , testCase "binary" $ do
        let bs :: LBS.ByteString
            bs = "\0\1\2\3\0\1\2\3"

        -- print $ LBS.unpack bs
        -- print $ LBS.unpack $(TH.lift $ LBS.pack [0,1,2,3,0,1,2,3])

        bs @=? $(TH.lift $ LBS.pack [0,1,2,3,0,1,2,3])

is very trivial. I'm not sure whom to blame, bytestring or GHC. If I run test-suite with -O0 i.e. without optimizations, it passes.

@phadej
Copy link
Contributor Author

phadej commented May 22, 2021

I see

Rule fired:
    ByteString packChars/packAddress (Data.ByteString.Internal)

firign, but it IMHO should NOT fire on (Modified-UTF8) encoded "\0\1\2\3\0\1\2\3". @bgamari Can you check this.

The test suite passes on GHC-8.10.4 and older, so I think its GHC issue.

EDIT: sorry, it was for "foobar", so it's not the culprit.

@phadej
Copy link
Contributor Author

phadej commented May 22, 2021

I opened #393 for literal issue, I can reproduce it with master version (and the one bundled with GHC-9.2), so it's not due this patch.

@phadej phadej force-pushed the th-lift branch 2 times, most recently from 74d764b to 9dd65a0 Compare May 31, 2021 17:35
@phadej
Copy link
Contributor Author

phadej commented May 31, 2021

I don't know what's wrong with windows job, but it seem to cancel everything else?

@phadej
Copy link
Contributor Author

phadej commented Jun 1, 2021

CI green.

@phadej phadej marked this pull request as ready for review June 1, 2021 11:10
@sjakobi
Copy link
Member

sjakobi commented Jun 1, 2021

have to add SBS instance still.

Do you want to do this in this PR?

@phadej phadej force-pushed the th-lift branch 4 times, most recently from f049b72 to 38109e3 Compare June 1, 2021 14:20
@phadej
Copy link
Contributor Author

phadej commented Jun 1, 2021

@sjakobi thanks for the reminder. ShortByteString instance added.

`TH.appE` TH.litE (TH.BytesPrimL $ TH.Bytes ptr 0 (fromIntegral len))
#else
lift bs@(BS _ len) = [| unsafePackLenLiteral |]
`TH.appE` TH.litE (TH.integerL (fromIntegral len))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we possibly have less duplication? It seems that only the last line is conditional on template-haskell version. Same for SBS.

Copy link
Contributor Author

@phadej phadej Jun 1, 2021

Choose a reason for hiding this comment

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

it's not the same,

lift (BS ptr len) = ...

vs

lift bs@(BS _ len) = ...

Disable unused variable warning and I will simpify.

@@ -165,6 +165,15 @@ test-suite test-builder
ghc-options: -Wall -fwarn-tabs -threaded -rtsopts
default-language: Haskell2010

test-suite bytestring-th
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put this into one of existing test suites. Linking an additional executable is expensive on Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In which of three it belongs? lazy-hclose and test-builder seems to be testing specific features, and prop-compiled is for properties. It looks like fourth test-suite for an isolated feature is following the example.

tests/bytestring-th.hs Show resolved Hide resolved
Copy link
Contributor

@Bodigrim Bodigrim left a comment

Choose a reason for hiding this comment

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

Looks good, I'll merge all test suites into one executable later.

@Bodigrim Bodigrim requested a review from sjakobi June 13, 2021 17:50
@Bodigrim Bodigrim linked an issue Jun 13, 2021 that may be closed by this pull request
@Bodigrim Bodigrim added this to the 0.11.2.0 milestone Jun 15, 2021
@Bodigrim Bodigrim merged commit 77ca061 into haskell:master Jun 15, 2021
@phadej phadej deleted the th-lift branch June 15, 2021 23:59
@phadej
Copy link
Contributor Author

phadej commented Oct 10, 2021

When the next release will be made? Will bytestring-0.11.2.0 be bundled with GHC-9.2? EDIT: or even GHC-9.0.2?

@Bodigrim
Copy link
Contributor

Feature freeze for GHC 9.2 was half a year ago and RC is out, so I expect it to stick with bytestring-0.11.1.0, even if we make a new release right now. GHC 9.0 remains on bytestring-0.10 series.

My plan is to release bytestring-0.11.2.0 either by feature freeze for GHC 9.4, or once #365 and #423 are merged, whatever earlier.

@phadej
Copy link
Contributor Author

phadej commented Oct 10, 2021

bytestring-0.11.2.0 is a minor release, no breaking changes. Why can't it be included? It definitely can in GHC-9.0.2, and I definitely would like to have this patch in GHC-9.2 (.1 or .2) too.

@phadej
Copy link
Contributor Author

phadej commented Oct 10, 2021

In particular: https://gitlab.haskell.org/ghc/ghc/-/wikis/commentary/libraries/version-history

bytestring was updated in GHG-8.10 series, also containers, haskeline and process.

@Bodigrim
Copy link
Contributor

Bodigrim commented Oct 10, 2021

It's not like I'm against making new developments of bytestring available to users without a year or two of delay, right? I'm relaying my understanding of situation according to previous discussions with GHC team, I'd be happy to learn that we can make new bytestrings available earlier.

@phadej
Copy link
Contributor Author

phadej commented Oct 10, 2021

ping @bgamari @emilypi

@phadej
Copy link
Contributor Author

phadej commented Nov 22, 2021

Can bytestring-0.11.2.0 be released now as GHC-9.2.1 is released. (And possibly included with GHC-9.2.2?)

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.

Add Lift ByteString instances
3 participants