findIndexEnd

[strake]

No description provided.

[strake]

ping

[strake]

ping

[strake]

ping

[strake]

ping

[sjakobi]

I think this looks very solid! A few requests though:

1. Add complexity annotations.
2. Add tests, ideally property tests.
3. Check whether we can refactor elemIndexEnd to use findIndexEnd and still get the same core.

[sjakobi]

I'm not sure how efficient this is, but it's very similar to the existing elemIndexEnd, so at least it shouldn't be worse.

bytestring/Data/ByteString/Lazy.hs

Lines 909 to 925 in 95fe6bd

	-- | /O(n)/ The 'elemIndexEnd' function returns the last index of the
	-- element in the given 'ByteString' which is equal to the query
	-- element, or 'Nothing' if there is no such element. The following
	-- holds:
	--
	-- > elemIndexEnd c xs ==
	-- > (-) (length xs - 1) `fmap` elemIndex c (reverse xs)
	--
	-- @since 0.10.6.0
	elemIndexEnd :: Word8 -> ByteString -> Maybe Int64
	elemIndexEnd w = elemIndexEnd' 0
	where
	elemIndexEnd' _ Empty = Nothing
	elemIndexEnd' n (Chunk c cs) =
	let !n' = n + S.length c
	!i = fmap (fromIntegral . (n +)) $ S.elemIndexEnd w c
	in elemIndexEnd' n' cs `mplus` i

[sjakobi]

This is very similar to elemIndexEnd, although some style and variable name differences make that a bit hard to see.

bytestring/Data/ByteString.hs

Lines 1092 to 1109 in 95fe6bd

	-- | /O(n)/ The 'elemIndexEnd' function returns the last index of the
	-- element in the given 'ByteString' which is equal to the query
	-- element, or 'Nothing' if there is no such element. The following
	-- holds:
	--
	-- > elemIndexEnd c xs ==
	-- > (-) (length xs - 1) `fmap` elemIndex c (reverse xs)
	--
	elemIndexEnd :: Word8 -> ByteString -> Maybe Int
	elemIndexEnd ch (PS x s l) = accursedUnutterablePerformIO $ withForeignPtr x $ \p ->
	go (p `plusPtr` s) (l-1)
	where
	go !p !i | i < 0 = return Nothing
	| otherwise = do ch' <- peekByteOff p i
	if ch == ch'
	then return $ Just i
	else go p (i-1)
	{-# INLINE elemIndexEnd #-}

[hvr]

The reason I was hesitating about this new addition is that it tends to increase the surface API of bytestring; and for text which is effectively bytestring's cousin we have the policy to grow the surface API only when there's good motivation.

Moreover, this PR is a bit assymetric as it only extends the non-Char8 version.

[sjakobi]

Given that elemIndex has an End variant, and given that findIndex is a very simple generalization of elemIndex, I think it's just natural and consistent to add findIndexEnd too.

Since Char8.findIndexEnd is so trivial to define in terms of the Word8 version, I'm not sure about the value of adding it too. But of course, consistency is always nice.

BTW Data.ByteString.Lazy.Char8 doesn't have elemIndexEnd, as noted in #104. I'm not sure whether I should read that issue as a feature request though.

[strake]

Indeed, given we have findIndex, elemIndex, and elemIndexEnd, it was an unpleasant surprise we lack findIndexEnd.

[sjakobi]

The reason I was hesitating about this new addition is that it tends to increase the surface API of bytestring; and for text which is effectively bytestring's cousin we have the policy to grow the surface API only when there's good motivation.

While I don't think that API considerations for text should necessarily be an important concern in decisions on the bytestring API, I'd like to point out that findIndexEnd has also been requested for Text:

haskell/text#243

[Bodigrim]

I second that it's worth to fix assymetry between elemIndex / findIndex / elemIndexEnd / missing findIndexEnd. Let us merge this and raise a separate issue about similar changes for Char8 version.

[Bodigrim]

@strake care to add changelog entries please?

[strake]

@Bodigrim Done

[Bodigrim]

@strake sorry for confusion, 0.10.10.1 has not been released yet, so please put your entry under it.

@sjakobi are you satisfied with changes after your review?

[strake]

@Bodigrim I believe this needs a minor (rather than patch) version bump, per PVP.

[Bodigrim]

I agree, I think it is fair to change 0.10.10.1 to 0.10.X.X and the rest of the line to TBA, since I believe @hvr will make a release only at some point in future.

[strake]

@Bodigrim I'm not sure whether you're asking me to bump the version in the changelog or implying someone else will do so and i should then merge/rebase.

[Bodigrim]

I'm asking you to change the existing header in Changelog.md from 0.10.10.1 ... to 0.10.X.X TBA and put your entry under this section.

There was no 0.10.10.1 release, and the next version number will be up to @hvr to decide, when the time is right. So let's put 0.10.X.X for now.

[sjakobi]

Since I was interested in the effect of the refactoring of elemIndexEnd, I compared the old Core with the one produced by this patch:

--- RHS size: {terms: 102, types: 81, coercions: 0, joins: 3/5}
-elemIndexEnd
-  = \ ch ds ->
-      case ds of { PS dt dt1 dt2 dt3 ->
-      let { ww = plusAddr# dt dt2 } in
-      let { ww1 = -# dt3 1# } in
+-- RHS size: {terms: 102, types: 79, coercions: 0, joins: 3/5}
+$welemIndexEnd
+  = \ w ww ww1 ww2 ww3 ->
+      let { ww4 = plusAddr# ww ww2 } in
+      let { ww5 = -# ww3 1# } in
       join {
-        exit ww2 ipv
-          = case touch# dt1 ipv of { __DEFAULT -> Just (I# ww2) } } in
-      join { exit1 w = case touch# dt1 w of { __DEFAULT -> Nothing } } in
-      case <# ww1 0# of {
+        exit ww6 ipv
+          = case touch# ww1 ipv of { __DEFAULT -> Just (I# ww6) } } in
+      join {
+        exit1 w1 = case touch# ww1 w1 of { __DEFAULT -> Nothing } } in
+      case <# ww5 0# of {
         __DEFAULT ->
-          case readWord8OffAddr# (plusAddr# ww ww1) 0# realWorld# of
+          case readWord8OffAddr# (plusAddr# ww4 ww5) 0# realWorld# of
           { (# ipv, ipv1 #) ->
-          case ch of { W8# x ->
+          case w of { W8# x ->
           case eqWord# x ipv1 of {
             __DEFAULT ->
               joinrec {
-                $wgo2 @ b ww2 ww3 w
-                  = case <# ww3 0# of {
+                $wgo2 @ b ww6 ww7 w1
+                  = case <# ww7 0# of {
                       __DEFAULT ->
-                        case readWord8OffAddr# (plusAddr# ww2 ww3) 0# w of
+                        case readWord8OffAddr# (plusAddr# ww6 ww7) 0# w1 of
                         { (# ipv2, ipv3 #) ->
                         case eqWord# x ipv3 of {
-                          __DEFAULT -> jump $wgo2 ww2 (-# ww3 1#) ipv2;
-                          1# -> jump exit ww3 ipv2
+                          __DEFAULT -> jump $wgo2 ww6 (-# ww7 1#) ipv2;
+                          1# -> jump exit ww7 ipv2
                         }
                         };
-                      1# -> jump exit1 w
+                      1# -> jump exit1 w1
                     }; } in
-              jump $wgo2 ww (-# ww1 1#) ipv;
-            1# -> jump exit ww1 ipv
+              jump $wgo2 ww4 (-# ww5 1#) ipv;
+            1# -> jump exit ww5 ipv
           }
           }
           };
         1# -> jump exit1 realWorld#
       }
+
+-- RHS size: {terms: 11, types: 7, coercions: 0, joins: 0/0}
+elemIndexEnd
+  = \ w w1 ->
+      case w1 of { PS ww1 ww2 ww3 ww4 ->
+      $welemIndexEnd w ww1 ww2 ww3 ww4
+      }

Apart from a few changed variable names, the main effect is that elemIndexEnd is split into a wrapper around a worker function $welemIndexEnd. That seems alright to me. :)

[sjakobi]

LGTM modulo the changelog entry. :)

Thanks @strake!

[strake]

I'm asking you to change the existing header in Changelog.md from 0.10.10.1 ... to 0.10.X.X TBA and put your entry under this section.

@Bodigrim Done

[Bodigrim]

@hvr @cartazio could we please have this merged?

[sjakobi]

This looks ready to go to me.

@hsyl20 Would you like to review?

[hsyl20]

Perhaps add INLINABLE pragmas to elemIndexEnd to ensure that they are exported.

Otherwise it looks good to me.

[strake]

@hsyl20 done

[Bodigrim]

Thanks, @strake!