-
Notifications
You must be signed in to change notification settings - Fork 426
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 some problems in sets / hash tables of arrays #18640
Conversation
Specifically, we were using `key == key` before, but for an array type, this will generate an array of bools rather than a single bool as we require here. So push the test into a helper routine to compute it. --- Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
--- Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
--- Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
Tagging:
One thought I have as I type this is whether we should also have ChapelHashtable.chpl warn if one uses an array key type today to avoid this valgrind-like unpredictable behavior (?). |
Aha, cool, thanks Michael. When we were talking about #15929 yesterday, I was misunderstanding that it might require users to apply the workaround proposed in that issue to their code—not that we could apply it within |
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
--- Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
…nto fix-set-of-arr
--- Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
OK, I think this PR is ready for review, thanks again Michael. Would one of @mppf, @daviditen, or @bmcdonald3 be willing to review it, or should I go fishing? |
I don't mind taking a look, though, I likely won't be able to until near the end of the day |
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.
Looks good, only one question above. The only other comment I have is that the 2 tests seem very similar, though, no reason to not have more tests I suppose.
@@ -343,6 +343,15 @@ module ChapelHashtable { | |||
|
|||
// #### add & remove helpers #### | |||
|
|||
// a utility to check for key equality | |||
proc keysMatch(key1: ?t, key2: t) { |
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.
Just since we are adding this helper function, I am curious: are there any other special cases besides arrays that should be considered? (Maybe domains or something that I wouldn't know enough about to point out)
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 don't believe there are, though am not entirely confident myself (and am telling myself "Well, at least I made it no worse", though you're doing the correct job as a reviewer of asking).
I think we've asked whether collections like lists should return a bool or a list of bools, but today list returns bools, and I'm not able to come up with other types that don't return bool.
That's a good point and only reflects the fact that I originally thought one of them was going to have to remain a future. I'll combine them (the reason for not having more tests unnecessarily when it's trivial to combine them being testing time). |
Keeping two separate tests made more sense when one of them was a future. Now it just seems like a waste of compilation time. --- Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
This fixes support for sets of arrays by fixing a
==
-based key comparisonand using an explicit
new
call to create the underlying hash table ratherthan relying on default initialization (contributed by @mppf).
Specifically, we were using
key == key
to look for existing keys before, butfor an array type, this will generate an array of bools rather than a single bool
as we require here. So this PR pushes the test into a helper routine to compute
it, which is sufficient to get the setOfArray.chpl future working.
I then extended setOfArray.chpl to see if we could actually work with the set,
and it turned out that we couldn't due to #15929 until we applied the workaround
in that issue to ensure that the set's element type's runtime type was established
properly (thanks to @mppf).
While working on this, I added a pair of futures that demonstrate that a generic
record instantiated with an array doesn't work:
genericRecordOfArray*.chpl
which seem related to:
in
intent #13808 and/orbut felt simpler than any futures I quickly found for those cases.
Resolves #16033.