-
Notifications
You must be signed in to change notification settings - Fork 178
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
add fromList tests #663
add fromList tests #663
Conversation
-- | ||
-- Uses `reallyUnsafePtrEquality#`. | ||
isUnit :: () -> Bool | ||
isUnit = same () |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering whether I'm reinventing a wheel here. This seems closely related to what ChasingBottoms is doing, but I didn't find anything like this there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This smells funny/fragile to me. I would generally feel more comfortable with an approach that uses unsafePerformIO
to find out when/how many times things are evaluated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, but of course we can't always do that .... Hrmmm. Could you at least rename same
to match our internal pointer equality function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I wasn't aware of ptrEq
, or I would have used that; will change.
I should probably make the tests conditional on __GLASGOW_HASKELL__
being defined... and maybe make isUnit tristate? (Yes | No | Maybe; the last indicating that the result is unknown.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I wasn't aware of
ptrEq
, or I would have used that; will change.
Ah it's not so easy because the module is hidden, and we probably want to keep the library parts of containers-tests
and containers
the same.
I should probably make the tests conditional on
__GLASGOW_HASKELL__
being defined... and maybe make isUnit tristate? (Yes | No | Maybe; the last indicating that the result is unknown.)
I will go with Maybe Bool
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given your later comment, I stuck with Bool
. I did add some logic to deal with non-ghc implementations though.
Do you consider the test or the code correct? Or neither? |
I believe the test is correct (it will pass once #658 is merged). |
Can you rebase please?
…On Sun, Jul 14, 2019, 5:41 PM Bertram Felgenhauer ***@***.***> wrote:
I believe the test is correct (it will pass once #658
<#658> is merged).
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#663?email_source=notifications&email_token=AAOOF7P3SMSG4PCCWRFTSNTP7OMPTA5CNFSM4H6667HKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZ4N36A#issuecomment-511237624>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAOOF7ISDKWPARUR7FJ43H3P7OMPTANCNFSM4H6667HA>
.
|
Done. (I don't know whether the force-push itself causes a notification.) |
The test suite is already pretty GHC-specific, so I wouldn't worry about
that. Tristate ... not sure. Depends what you're trying to check. Add more
comments?
…On Sun, Jul 14, 2019, 9:46 PM Bertram Felgenhauer ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In containers-tests/tests/Utils/IsUnit.hs
<#663 (comment)>:
> +{-# LANGUAGE MagicHash #-}
+
+module Utils.IsUnit (isUnit) where
+
+import GHC.Exts
+
+same :: a -> a -> Bool
+same x y = case reallyUnsafePtrEquality# x y of
+ 0# -> False
+ _ -> True
+
+-- | Check whether the argument is a fully evaluated unit `()`.
+--
+-- Uses `reallyUnsafePtrEquality#`.
+isUnit :: () -> Bool
+isUnit = same ()
Oh I wasn't aware of ptrEq, or I would have used that; will change.
I should probably make the tests conditional on *GLASGOW_HASKELL* being
defined... and maybe make isUnit tristate? (Yes | No | Maybe; the last
indicating that the result is unknown.)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#663?email_source=notifications&email_token=AAOOF7JEXKJCY4WTQVETO63P7PJIBA5CNFSM4H6667HKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB6L6IRA#discussion_r303272752>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAOOF7MOBKPA54D5G7WIGOLP7PJIBANCNFSM4H6667HA>
.
|
Nothing new from my side... the branch rebased cleanly on current master, and the tests currently pass (for me). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! I see a few wibbles, but nothing big. I hope we can get this merged soon! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One last wibble.
These tests look really useful to me!
- the function is rather peculiar in that in the presence of duplicate keys, the first value is not evaluated, but all the others are evaluated. See also #473.
- we check most functions that create or update entries, namely singleton, insert, insertWith, fromList, fromListWith, fromAscList, fromDistinctAscList, and fromAscListWith
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM and many thanks! :)
@treeowl, please review!
This is adding some test cases that were previously part of #658.
Note that one test is currently failing because
Data.IntMap.fromAscList
is a bit lazier than expected.