Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8591022:
Make HashMap's delete key a const expression. (Closed)
|
Created:
9 years, 1 month ago by Siggi Cherem (dart-lang) Modified:
9 years, 1 month ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionMake HashMap's delete key a const expression.
Committed: https://code.google.com/p/dart/source/detail?r=1661
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : r #
Total comments: 19
Patch Set 4 : ivan comments #Patch Set 5 : '' #
Total comments: 6
Messages
Total messages: 8 (0 generated)
I'm not 100% sure about whether this change is ok. I ran ./tools/presubmit.py and all tests pass. If I could do this change, it will simplify the isolate code generation in frog since it'll allow us to use maps before we fully initialize isolates. http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... File corelib/src/implementation/hash_map_set.dart (right): http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:40: static final Object _DELETED_KEY = const _DeleteKeySentinel(); Is there an issue in using an instance that is not [Object], but [_DeleteKeySentinel] here?
http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... File corelib/src/implementation/hash_map_set.dart (right): http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:11: // [_DeleteKeySentinel]. DeletedKeySentinel http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:40: static final Object _DELETED_KEY = const _DeleteKeySentinel(); On 2011/11/17 18:40:32, sigmund wrote: > Is there an issue in using an instance that is not [Object], but > [_DeleteKeySentinel] here? Why is there even a type on this value? I don't think it needs to be there and if you want to specify a type then you should use _DeletedKeySentinel. http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:53: // See bug 5257789. This bug is claiming to be fixed? http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:79: Object existingKey = _keys[hash]; How about change Object here to var and adding a comment as to why not all _keys are not necessarily of type K? http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:109: Object existingKey = _keys[hash]; ditto. http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:155: Object key = oldKeys[i]; ditto. http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:406: Object deletedKey = HashMapImplementation._DELETED_KEY; ditto: var or _DeletedKeySentinel http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:430: int hashCode() => 1; Why do you need the hashCode() method here at all? Object did not have it and you also should not need to implement Hashable.
thanks Ivan http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... File corelib/src/implementation/hash_map_set.dart (right): http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:11: // [_DeleteKeySentinel]. On 2011/11/17 18:57:47, Ivan Posva wrote: > DeletedKeySentinel Done (I expect you meant here to remove the _ from the type name). http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:40: static final Object _DELETED_KEY = const _DeleteKeySentinel(); On 2011/11/17 18:57:47, Ivan Posva wrote: > On 2011/11/17 18:40:32, sigmund wrote: > > Is there an issue in using an instance that is not [Object], but > > [_DeleteKeySentinel] here? > > Why is there even a type on this value? I don't think it needs to be there and > if you want to specify a type then you should use _DeletedKeySentinel. Done. http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:53: // See bug 5257789. On 2011/11/17 18:57:47, Ivan Posva wrote: > This bug is claiming to be fixed? Tried - works in the VM, but I still get some failures in the dartc tests. http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:79: Object existingKey = _keys[hash]; On 2011/11/17 18:57:47, Ivan Posva wrote: > How about change Object here to var and adding a comment as to why not all _keys > are not necessarily of type K? I added the comment, but left it as [Object] rather than [var]. I prefer to leave is as Object because it gives us better errors in checked mode: having Object says that even though we don't know the precise type, we don't plan to access anything on it and we should get a type error if we do. http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:109: Object existingKey = _keys[hash]; On 2011/11/17 18:57:47, Ivan Posva wrote: > ditto. Done. http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:155: Object key = oldKeys[i]; On 2011/11/17 18:57:47, Ivan Posva wrote: > ditto. Done. http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:406: Object deletedKey = HashMapImplementation._DELETED_KEY; On 2011/11/17 18:57:47, Ivan Posva wrote: > ditto: var or _DeletedKeySentinel Done. http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:430: int hashCode() => 1; On 2011/11/17 18:57:47, Ivan Posva wrote: > Why do you need the hashCode() method here at all? Object did not have it and > you also should not need to implement Hashable. removed. I had some failing tests while implementing static/globals in frog isolates and this is something I was playing with to help me debug. I didn't mean to leave it here.
http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... File corelib/src/implementation/hash_map_set.dart (right): http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:11: // [_DeleteKeySentinel]. On 2011/11/17 20:00:21, sigmund wrote: > On 2011/11/17 18:57:47, Ivan Posva wrote: > > DeletedKeySentinel > > Done (I expect you meant here to remove the _ from the type name). You really, really want to keep this type private otherwise you suffer the same problems as you did with const Object(). What I meant to say is that you want to rename the type to _DeletedKeySentinel (note the d) because it refers to the deleted key and not to the delete operation.
http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... File corelib/src/implementation/hash_map_set.dart (right): http://codereview.chromium.org/8591022/diff/4001/corelib/src/implementation/h... corelib/src/implementation/hash_map_set.dart:11: // [_DeleteKeySentinel]. On 2011/11/17 21:06:51, Ivan Posva wrote: > On 2011/11/17 20:00:21, sigmund wrote: > > On 2011/11/17 18:57:47, Ivan Posva wrote: > > > DeletedKeySentinel > > > > Done (I expect you meant here to remove the _ from the type name). > > You really, really want to keep this type private otherwise you suffer the same > problems as you did with const Object(). What I meant to say is that you want to > rename the type to _DeletedKeySentinel (note the d) because it refers to the > deleted key and not to the delete operation. Fixed - I was assuming that because this type is within corelib_impl (and not corelib), it wasn't accessible elsewhere anyways.
LGTM -ip
LGTM http://codereview.chromium.org/8591022/diff/9/corelib/src/implementation/hash... File corelib/src/implementation/hash_map_set.dart (right): http://codereview.chromium.org/8591022/diff/9/corelib/src/implementation/hash... corelib/src/implementation/hash_map_set.dart:53: // See bug 5257789. Works in the vm, fails in dartc End comment with '.'. What about frog? http://codereview.chromium.org/8591022/diff/9/corelib/src/implementation/hash... corelib/src/implementation/hash_map_set.dart:79: // Keys can be either of type [K] or [_DeletedKeySentinel]. I wouldn't duplicate this comment all around (I guess that's for explaining with existingKey is Object). It's already stated at the _keys declaration. No strong opinion though. http://codereview.chromium.org/8591022/diff/9/corelib/src/implementation/hash... corelib/src/implementation/hash_map_set.dart:429: * canonicalized Object(). [: Object() :]
Thanks Ivan! Thanks Nicolas! submitting... http://codereview.chromium.org/8591022/diff/9/corelib/src/implementation/hash... File corelib/src/implementation/hash_map_set.dart (right): http://codereview.chromium.org/8591022/diff/9/corelib/src/implementation/hash... corelib/src/implementation/hash_map_set.dart:53: // See bug 5257789. Works in the vm, fails in dartc On 2011/11/18 08:31:01, ngeoffray wrote: > End comment with '.'. What about frog? Done. Just double checked, frog also fails. http://codereview.chromium.org/8591022/diff/9/corelib/src/implementation/hash... corelib/src/implementation/hash_map_set.dart:79: // Keys can be either of type [K] or [_DeletedKeySentinel]. On 2011/11/18 08:31:01, ngeoffray wrote: > I wouldn't duplicate this comment all around (I guess that's for explaining with > existingKey is Object). It's already stated at the _keys declaration. No strong > opinion though. Thx. Made the comment more specific to [existingKey]. http://codereview.chromium.org/8591022/diff/9/corelib/src/implementation/hash... corelib/src/implementation/hash_map_set.dart:429: * canonicalized Object(). On 2011/11/18 08:31:01, ngeoffray wrote: > [: Object() :] Done. |
