Implementation of IReadOnlyDictionary on GroupCollection by peltco · Pull Request #30077 · dotnet/corefx

peltco · 2018-06-02T11:55:45Z

Here is my implementation of IReadOnlyDictionary on GroupCollection. Any feedback on this pullrequest would be apreciated

Fixes https://github.com/dotnet/corefx/issues/23262

dnfclas · 2018-06-02T11:55:55Z

All CLA requirements met.

stephentoub · 2018-06-02T12:49:50Z

+        {
+            if (ContainsKey(key))
+            {
+                value = this[key];


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; }

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

stephentoub · 2018-06-02T12:51:32Z

+        {
+            get
+            {
+                return this._groups;


@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?

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.

stephentoub · 2018-06-02T12:51:59Z

            void IDisposable.Dispose() { }
        }
+
+        public bool TryGetValue(string key, out Group value)


Nit: all of the new methods should be moved up above the nested enumerator types.

stephentoub · 2018-06-02T12:53:21Z

+            object IEnumerator.Current => Current;
+        }
+
+        IEnumerator<KeyValuePair<string, Group>> IEnumerable<KeyValuePair<string, Group>>.GetEnumerator()


Nit: this should be moved up above the nested types

stephentoub · 2018-06-02T12:56:29Z

+
+            public EnumeratorKeyValue(GroupCollection collection)
+            {
+                _collectionEnumerator = new Enumerator(collection);


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.

I changed this. Normally i wouldn't think twice about a memory allocation. Interesting to see there are places where that actually does matter.

hughbe · 2018-06-02T16:03:02Z

+            }
+            else
+            {
+                Assert.True(false, "Value should exist");


Bit odd: maybe write something like

Assert.True(groups.TryGetValue("O", out Group value)); Assert.Null(value)

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)

ViktorHofer · 2018-06-02T18:05:33Z

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).

peltco · 2018-06-02T18:09:35Z

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

ViktorHofer · 2018-06-02T21:57:03Z

+                    if (_index < 0 || _index >= _collection.Count)
+                        throw new InvalidOperationException(SR.EnumNotStarted);
+
+                    var value = _collection[_index];


nit: concrete type instead of var here

I'll change that in the morging

ViktorHofer · 2018-06-02T21:58:25Z

+        {
+            get
+            {
+                var enumerator = new Enumerator(this);


@stephentoub does that seem right to you? Now we are allocating because of the enumerator instantiation, which is odd to me.

Yeah but if you don't want to risk exposing something internally the enumerator allocation seemed the cheapest alternative

ping @stephentoub

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.

I'm fine with the allocation. This is absolutely not a hot path.

stephentoub · 2018-06-04T16:51:18Z

+
+        public bool TryGetValue(string key, out Group value)
+        {
+            var group = this[key];


Nit: var => Group

stephentoub · 2018-06-04T16:56:07Z

+                    yield return enumerator.Current.Name;
+                }
+            }
+        }


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?

stephentoub · 2018-06-04T17:07:23Z

+                    return new KeyValuePair<string, Group>(value.Name, value);
+
+                }
+            }


Why is this and the additional interface implementation necessary?

@stephentoub because IReadOnlyDictionary requires a GetEnumerator that returns an enumerator that implements this interface. And this way we have 1 enumerator that implements both.

michael-hawker · 2018-06-04T21:52:33Z

+            }
+            else
+            {
+                Assert.True(false, "Value should exist");


Does xUnit not have an Assert.Fail method?

I couldn't find it. See also https://stackoverflow.com/questions/14631923/xunit-net-cannot-find-assert-fail-and-assert-pass-or-equivalent

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?

xunit has rejected this specific api proposal couple of times. We settled with just doing it this way...

Feel free to post on xunit repo though

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);

?

peltco · 2018-06-07T09:18:11Z

@stephentoub I fixed the code based on your review.

stephentoub · 2018-06-11T20:29:52Z

+            else
+            {
+                Assert.Null(value);
+            }


Similarly, can't this just be:

Assert.False(groups.TryGetValue("INVALID", out Group value)); Assert.Null(value);

?

stephentoub · 2018-06-11T20:30:24Z

+            Regex regex = new Regex(@"(?<A1>a*)(?<A2>b*)(?<A3>c*)");
+            Match match = regex.Match("aaabbccccccccccaaaabc");
+
+            var groups = match.Groups;


Nit: var => whatever type this is... same for the other occurrences of this in the other tests below

stephentoub · 2018-06-11T20:30:54Z

+            else
+            {
+                Assert.Null(value);
+            }


Ditto:

Assert.False(groups.TryGetValue("A1", out Group Value)); Assert.Null(value);

stephentoub · 2018-06-11T20:31:22Z

+            else
+            {
+                Assert.True(false, "Value should exist");
+            }


Ditto:

Assert.True(groups.TryGetValue("0", out Group value)); Assert.NotNull(value); Assert.True(value.Success);

stephentoub · 2018-06-11T20:31:38Z

+
+            var groups = match.Groups;
+
+            var keys = groups.Keys.ToArray();


Nit: var => whatever type this is

stephentoub · 2018-06-11T20:32:08Z

+            Assert.Equal(4, keys.Length);
+            Assert.Equal("A1", keys[1]);
+            Assert.Equal("A2", keys[2]);
+            Assert.Equal("A3", keys[3]);


Shouldn't keys[0] be validated, too?

stephentoub · 2018-06-11T20:32:33Z

+
+            var groups = match.Groups;
+
+            var values = groups.Values.ToArray();


stephentoub · 2018-06-11T20:32:42Z

+            Assert.Equal("bbb", values[2].Value);
+
+            Assert.Equal("A3", values[3].Name);
+            Assert.Equal("ccc", values[3].Value);


Same question about values[0]

stephentoub · 2018-06-11T20:32:53Z

+            Match match = regex.Match("aaabbccccccccccaaaabc");
+
+            IReadOnlyDictionary<string, Group> groups = match.Groups;
+            var enumerator = groups.GetEnumerator();


Nit: var => whatever type it is

stephentoub · 2018-06-11T20:33:11Z

+                {
+                    var result = enumerator.Current;
+                    Assert.NotNull(result);
+                    var group = match.Groups[counter];


Nit here and above: var => whatever types these are

stephentoub · 2018-06-11T20:34:01Z

+            var groupZero = groups[0];
+            Assert.Equal("aaabbbccc", groupZero.Value);
+
+            var groupC = groups[3];


Nit: all of these vars => whatever type they are

stephentoub

Left some more comments, otherwise LGTM

re-implementation based on comments in pullrequests

stephentoub · 2018-06-13T17:30:52Z

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

peltco changed the title ~~Implementation of IReadOnlyDictionary to GroupCollection~~ Implementation of IReadOnlyDictionary on GroupCollection Jun 2, 2018

stephentoub reviewed Jun 2, 2018

View reviewed changes

hughbe reviewed Jun 2, 2018

View reviewed changes

karelz assigned peltco Jun 2, 2018

karelz added the area-System.Text.RegularExpressions label Jun 2, 2018

karelz assigned ViktorHofer Jun 2, 2018

karelz added the Hackathon Issues picked for Hackathon label Jun 2, 2018

ViktorHofer reviewed Jun 2, 2018

View reviewed changes

ViktorHofer approved these changes Jun 4, 2018

View reviewed changes

stephentoub reviewed Jun 4, 2018

View reviewed changes

michael-hawker reviewed Jun 4, 2018

View reviewed changes

stephentoub reviewed Jun 11, 2018

View reviewed changes

peltco and others added 10 commits June 13, 2018 11:03

Implementation of IReadOnlyDictionary to GroupCollection

94697bb

add failing unittest

ed6fdf3

moved code around

c391d6b

update enumerator/only 1 allocation and slight code duplication

8be9cea

added some extra tests

884ae1b

re-implementation based on comments in pullrequests

fix style issue

648e57b

fix code review comments

15463c1

code review

9a7aa97

this[i] just calls Group(i)

aa9e734

Address my own PR feedback

568b19e

stephentoub force-pushed the issues/23262 branch from f17cd9c to 568b19e Compare June 13, 2018 16:00

stephentoub merged commit 7d945a0 into dotnet:master Jun 13, 2018

karelz added this to the 3.0 milestone Jul 8, 2018


		var groups = match.Groups;

		var values = groups.Values.ToArray();

Conversation

peltco commented Jun 2, 2018 • edited by ViktorHofer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnfclas commented Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViktorHofer commented Jun 2, 2018

Uh oh!

peltco commented Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peltco Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peltco commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peltco commented Jun 2, 2018 •

edited by ViktorHofer

Loading

dnfclas commented Jun 2, 2018 •

edited

Loading

peltco commented Jun 2, 2018 •

edited

Loading

peltco Jun 4, 2018 •

edited

Loading