Implement stimes more efficiently

[elikoga]

As requested in #299

I didn't write any test cases yet

[Bodigrim]

stimes for a negative argument should throw an error similar to stimesDefault.

Is strict stimes correct for n = 0?

[elikoga]

Is strict stimes correct for n = 0?

I've just taken the liberty to throw an error on any non-negative input to stimes just like stimesDefault.

[Bodigrim]

Sorry for confusion, I should have pointed you to stimesMonoid. It is desirable to handle n = 0 when we are dealing with a Monoid.

[Bodigrim]

Thanks, looks reasonable to me. Please go ahead with tests. It's enough to check that stimes and mtimesDefault are observably equivalent.

[elikoga]

Any Idea on how to get to stimesDefault or stimesMonoid without reimplementing them?

[Bodigrim]

http://hackage.haskell.org/package/base-4.14.0.0/docs/Data-Semigroup.html#v:mtimesDefault

[elikoga]

mtimesDefault n x
  | n == 0    = mempty
  | otherwise = unwrapMonoid (stimes n (WrapMonoid x))

I'm afraid it's implemented using stimes which wouldn't help us at all

[elikoga]

Oh just realized its using stimes of the wrapped monoid nvm

[elikoga]

Doesn't look like the strict version is working

[elikoga]

Turns out memcpy copies to the first argument, my mistake

[Bodigrim]

Looks very nice, thanks! Please fix tests for GHC < 8.0 by adding semigroups to build-depends of the test suite.
https://travis-ci.org/github/haskell/bytestring/jobs/734551109#L836

[Bodigrim]

Ah, that's me being stupid. Since bytestring library cannot depend on semigroups and prior to GHC 8.0 Semigroup was not in base, there is simply no instance Semigroup ByteString for GHC < 8.0. Could you please guard new test properties by CPP?

[elikoga]

Done

[Bodigrim]

@elikoga any chance to address @sjakobi's review suggestions? I'd like to get it merged soon. I can put finishing touches myself, if you are busy.

[elikoga]

@Bodigrim just did, sorry for the delay

[sjakobi]

A few more wibbles – nearly done.

[sjakobi]

Does returning BS fp len instead of a variable bound to the BS pattern result in additional allocations?

[elikoga]

Just checked with ddump-simpl and couldn't see any differences outside of different identifiers.
ddump-core-stats also returns the same amount of Terms, Types and Coercions, so I think it's optimized away. Wouldn't say no to assigning it to a name though if you believe it would be more readable

[elikoga]

The assembly generated with ddump-asm also looks essentially the same, up to renaming.

[sjakobi]

Thanks for investigating! 👍

[sjakobi]

Thank you, @elikoga! :)

[Bodigrim]

@elikoga last hurdle before merge: could you please fix tests? Seems Properties.hs lacks {-# LANGUAGE CPP #-}.
https://travis-ci.org/github/haskell/bytestring/jobs/739428587#L638

[elikoga]

>  head tests/Properties.hs
{-# LANGUAGE CPP, ScopedTypeVariables, BangPatterns #-}
--
-- Must have rules off, otherwise the rewrite rules will replace the rhs
-- with the lhs, and we only end up testing lhs == lhs
--

--
-- -fhpc interferes with rewrite rules firing.
--

I don't believe that Properties.hs has changed since c39c1e7 and I'm honestly quite confused what changes to make to fix it.
Do you have any idea?

[Bodigrim]

@elikoga Properties.hs was changed in 7968977. Try to rebase your branch atop of master please.

[elikoga]

Whoops my fault

[elikoga]

Looks like it works; If accepted please add the hacktoberfest-accepted label.

[Bodigrim]

Thanks, merged!