fix(map): ensure string-to-string flags maintain stable pointers#457
fix(map): ensure string-to-string flags maintain stable pointers#457tomasaschan merged 1 commit intospf13:masterfrom
Conversation
|
|
|
/cc @tomasaschan 🚀 |
|
Agh I just noticed that the other |
brandon1024
force-pushed
the
string-to-string-fix
branch
from
f12754f to
a758f15
Compare
tomasaschan
left a comment
tomasaschan
left a comment
There was a problem hiding this comment.
While I agree that the tests you removed were incredibly hard to read and understand what they were testing - and the new ones are much better, thanks! - I think there's value in keeping the old ones around if only to show that they're still passing.
Would that be OK with you (even if they test the same thing as the new ones)? If you feel like cleaning them up in the follow-up PR you mentioned, that's fine of course.
No problem at all 👍 |
This revision tweaks the internals of the StringToString flag type to ensure that pointers are consistent across invocations of FlagSet.Parse(), Flag.Set(), and FlagSet.GetStringToString(). Prior to this change, each call to GetStringToString would allocate and return a new map. Although this provides nice encapsulation, it means that users cannot manipulate the map (in tests, for instance). To address this, GetStringToString now returns the map directly from Flag.Value. Set() now avoids new allocations as well, updating the existing map over allocating new ones. Documentation for all StringToString flags have been reworked for improved clarity. The tests for this flag type have been completely reworked. A small example demonstrating this flag type was also added.
brandon1024
force-pushed
the
string-to-string-fix
branch
from
a758f15 to
4b23295
Compare
|
This should be ready for final review :-) |
tomasaschan
left a comment
tomasaschan
left a comment
There was a problem hiding this comment.
Very nice, thank you for your contribution!
3 participants
This revision tweaks the internals of the StringToString flag type to ensure that pointers are consistent across invocations of FlagSet.Parse(), Flag.Set(), FlagSet.GetStringToString().
Prior to this change, each call to GetStringToString would allocate and return a new map. Although this provides nice encapsulation, it means that users cannot manipulate the map (in tests, for instance). To address this, GetStringToString now returns the map directly from Flag.Value. Set() now avoids new allocations as well, updating the existing map over allocating new ones.
Documentation for all StringToString flags have been reworked for improved clarity. The tests for this flag type have been completely reworked. A small example demonstrating this flag type was also added.
Fixes #455
Fixes #457