-
Notifications
You must be signed in to change notification settings - Fork 5k
Implementation of IReadOnlyDictionary on GroupCollection #30077
Conversation
{ | ||
if (ContainsKey(key)) | ||
{ | ||
value = this[key]; |
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.
Is this going to incur two lookups, one for ContainsKey and one for the indexer? I'm not familiar with the costs of those operations (@ViktorHofer?), but I'm wondering if the implementation would instead be better something like:
public bool TryGetValue(string key, out Group value)
{
Group g = this[key];
if (g != Group.s_emptyGroup)
{
value = g;
return true;
}
value = null;
return false;
}
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.
They way i implemented ContainsKey is that it doesn't do a full lookup of the value, it really only checks if the name exists (if it can resolve an index from GroupNumberFromName)
I'll see if i can make it a bit more efficent though
{ | ||
get | ||
{ | ||
return this._groups; |
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.
@ViktorHofer, are we ok returning this array directly? The caller could cast back to Group[]
and then mutate the array. What would happen in that case, and do we care?
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.
Good catch. I haven't thought about that. The collection only lives on the caller's site => it is returned when calling Regex.Matches. We don't accept the GroupCollection as a parameter on any of the existing APIs, therefore I think we are fine with this side effect.
But I don't have a strong opinion here. I would also be fine with a copy returned.
@@ -250,5 +250,69 @@ void IEnumerator.Reset() | |||
|
|||
void IDisposable.Dispose() { } | |||
} | |||
|
|||
public bool TryGetValue(string key, out Group value) |
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.
Nit: all of the new methods should be moved up above the nested enumerator types.
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.
fixed
object IEnumerator.Current => Current; | ||
} | ||
|
||
IEnumerator<KeyValuePair<string, Group>> IEnumerable<KeyValuePair<string, Group>>.GetEnumerator() |
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.
Nit: this should be moved up above the nested types
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.
fixed
|
||
public EnumeratorKeyValue(GroupCollection collection) | ||
{ | ||
_collectionEnumerator = new Enumerator(collection); |
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 means that enumerating this as an IReadOnlyDictionary will now incur two allocations instead of one. There's minimal logic in the existing enumerator; I think it'd be better to use copy that logic here, updated accordingly, to avoid that second enumerator allocation.
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 changed this. Normally i wouldn't think twice about a memory allocation. Interesting to see there are places where that actually does matter.
} | ||
else | ||
{ | ||
Assert.True(false, "Value should exist"); |
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.
Bit odd: maybe write something like
Assert.True(groups.TryGetValue("O", out Group value));
Assert.Null(value)
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.
Well of course that would be possible too. Why do you find it a bit odd though. I think they are rather equivalent. (I picked this because the trygetvalue inside an if would be how I would be using this code elsewhere)
Okay I digged through the sources and it seems we can't change the Hashtable to a Dictionary<int, int> in the GroupCollection easily because we expose the caps as a Hashtable in Regex (protected internal). |
Yeah I got that far as well ;-) I reworked my code a bit based of the feedback so far. Of people could take a look too see if I missed any issues I'd appreciate it |
if (_index < 0 || _index >= _collection.Count) | ||
throw new InvalidOperationException(SR.EnumNotStarted); | ||
|
||
var value = _collection[_index]; |
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.
nit: concrete type instead of var here
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'll change that in the morging
{ | ||
get | ||
{ | ||
var enumerator = new Enumerator(this); |
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.
@stephentoub does that seem right to you? Now we are allocating because of the enumerator instantiation, which is odd to 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.
Yeah but if you don't want to risk exposing something internally the enumerator allocation seemed the cheapest alternative
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.
ping @stephentoub
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.
Yeah but if you don't want to risk exposing something internally the enumerator allocation seemed the cheapest alternative
Right, either you expose the internal data structure or allocate a wrapper. It's up to you, @ViktorHofer.
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 fine with the allocation. This is absolutely not a hot path.
|
||
public bool TryGetValue(string key, out Group value) | ||
{ | ||
var group = this[key]; |
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.
Nit: var => Group
yield return enumerator.Current.Name; | ||
} | ||
} | ||
} |
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 has the same issue I previously mentioned, that there are two allocations here. The first allocation is for the Enumerable returned from Keys, which the compiler will then also use as the enumerator returned from its GetEnumerator call. The second allocation is for the new Enumerator(this)
above. You don't need both. Can't this instead just be implemented simply like:
public IEnumerable<Group> Keys
{
for (int i = 0; i < Count; i++)
{
yield return this[i].Name;
}
}
and similarly for Values?
return new KeyValuePair<string, Group>(value.Name, value); | ||
|
||
} | ||
} |
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.
Why is this and the additional interface implementation necessary?
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.
@stephentoub because IReadOnlyDictionary requires a GetEnumerator that returns an enumerator that implements this interface. And this way we have 1 enumerator that implements both.
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.
Ah. Ok.
} | ||
else | ||
{ | ||
Assert.True(false, "Value should exist"); |
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.
Does xUnit not have an Assert.Fail method?
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 couldn't find it. See also https://stackoverflow.com/questions/14631923/xunit-net-cannot-find-assert-fail-and-assert-pass-or-equivalent
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.
Ah, interesting. @ViktorHofer do you know if there's a reason why it's not there? Should I open an issue as a feature request for xUnit?
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.
xunit has rejected this specific api proposal couple of times. We settled with just doing it this way...
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.
Feel free to post on xunit repo though
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.
Why write it with this if/else and Assert.True(false, ...) rather than just doing:
Assert.True(groups.TryGetValue("A1", out Group value));
Assert.Equal("aaa", value.Value);
?
@stephentoub I fixed the code based on your review. |
else | ||
{ | ||
Assert.Null(value); | ||
} |
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.
Similarly, can't this just be:
Assert.False(groups.TryGetValue("INVALID", out Group value));
Assert.Null(value);
?
Regex regex = new Regex(@"(?<A1>a*)(?<A2>b*)(?<A3>c*)"); | ||
Match match = regex.Match("aaabbccccccccccaaaabc"); | ||
|
||
var groups = match.Groups; |
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.
Nit: var => whatever type this is... same for the other occurrences of this in the other tests below
else | ||
{ | ||
Assert.Null(value); | ||
} |
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.
Ditto:
Assert.False(groups.TryGetValue("A1", out Group Value));
Assert.Null(value);
else | ||
{ | ||
Assert.True(false, "Value should exist"); | ||
} |
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.
Ditto:
Assert.True(groups.TryGetValue("0", out Group value));
Assert.NotNull(value);
Assert.True(value.Success);
|
||
var groups = match.Groups; | ||
|
||
var keys = groups.Keys.ToArray(); |
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.
Nit: var => whatever type this is
Assert.Equal(4, keys.Length); | ||
Assert.Equal("A1", keys[1]); | ||
Assert.Equal("A2", keys[2]); | ||
Assert.Equal("A3", keys[3]); |
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.
Shouldn't keys[0] be validated, too?
|
||
var groups = match.Groups; | ||
|
||
var values = groups.Values.ToArray(); |
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.
Same nits
Assert.Equal("bbb", values[2].Value); | ||
|
||
Assert.Equal("A3", values[3].Name); | ||
Assert.Equal("ccc", values[3].Value); |
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.
Same question about values[0]
Match match = regex.Match("aaabbccccccccccaaaabc"); | ||
|
||
IReadOnlyDictionary<string, Group> groups = match.Groups; | ||
var enumerator = groups.GetEnumerator(); |
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.
Nit: var => whatever type it is
{ | ||
var result = enumerator.Current; | ||
Assert.NotNull(result); | ||
var group = match.Groups[counter]; |
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.
Nit here and above: var => whatever types these are
var groupZero = groups[0]; | ||
Assert.Equal("aaabbbccc", groupZero.Value); | ||
|
||
var groupC = groups[3]; |
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.
Nit: all of these vars => whatever type they are
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.
Left some more comments, otherwise LGTM
re-implementation based on comments in pullrequests
f17cd9c
to
568b19e
Compare
Thanks, @peltco. |
…fx#30077) * Implementation of IReadOnlyDictionary to GroupCollection * add failing unittest * moved code around * update enumerator/only 1 allocation and slight code duplication * added some extra tests re-implementation based on comments in pullrequests * fix style issue * fix code review comments * code review * this[i] just calls Group(i) * Address my own PR feedback Commit migrated from dotnet/corefx@7d945a0
Here is my implementation of IReadOnlyDictionary on GroupCollection. Any feedback on this pullrequest would be apreciated
Fixes https://github.com/dotnet/corefx/issues/23262