Skip to content
This repository has been archived by the owner on Oct 2, 2021. It is now read-only.

Fix some unexplained breakpoints behavior #296

Merged

Conversation

digeff
Copy link
Contributor

@digeff digeff commented Mar 2, 2018

If the breakpointId is a number, these two types of key can get incorrectly mixed with each other.

@digeff
Copy link
Contributor Author

digeff commented Mar 2, 2018

@roblourens

@digeff
Copy link
Contributor Author

digeff commented Mar 2, 2018

@changsi-an

@digeff digeff changed the base branch from feature/telemetry to master March 2, 2018 23:19
@digeff digeff changed the base branch from master to feature/telemetry March 2, 2018 23:20
@digeff digeff changed the base branch from feature/telemetry to master March 2, 2018 23:22
@digeff digeff force-pushed the fix_breakpoints_unexplained_behavior branch from 20798b5 to 3b7ddbd Compare March 2, 2018 23:23
@digeff
Copy link
Contributor Author

digeff commented Mar 2, 2018

@roblourens We want you to take a look before merging this, so we are retargeting this PR to master.

@roblourens
Copy link
Member

What's the problem exactly? I don't understand the description.

@roblourens
Copy link
Member

roblourens commented Mar 5, 2018

I think this is the wrong fix, the point of keeping unbound bps in this set of handles is that they are the same thing as the bound BPs, and the .set call is recording the change from an unbound BP to a bound BP. What happens when unbound breakpoints are bound and you try to look them up later? I think a better thing to do is to change the unbound ID to not conflict with a real id, like by adding _unbound to the number or something.

@digeff
Copy link
Contributor Author

digeff commented Mar 5, 2018

Applied feedback

@roblourens roblourens merged commit 51e8e5d into microsoft:master Mar 6, 2018
@digeff digeff deleted the fix_breakpoints_unexplained_behavior branch March 6, 2018 00:35
@roblourens roblourens added this to the March 2018 milestone Apr 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants