{"action":"created","issue":{"url":"https://api.github.com/repos/dimitar-asenov/Envision/issues/390","repository_url":"https://api.github.com/repos/dimitar-asenov/Envision","labels_url":"https://api.github.com/repos/dimitar-asenov/Envision/issues/390/labels{/name}","comments_url":"https://api.github.com/repos/dimitar-asenov/Envision/issues/390/comments","events_url":"https://api.github.com/repos/dimitar-asenov/Envision/issues/390/events","html_url":"https://github.com/dimitar-asenov/Envision/pull/390","id":157076232,"number":390,"title":"Introduce virtual breakpoint ids","user":{"login":"lukedirtwalker","id":3530769,"avatar_url":"https://avatars.githubusercontent.com/u/3530769?v=3","gravatar_id":"","url":"https://api.github.com/users/lukedirtwalker","html_url":"https://github.com/lukedirtwalker","followers_url":"https://api.github.com/users/lukedirtwalker/followers","following_url":"https://api.github.com/users/lukedirtwalker/following{/other_user}","gists_url":"https://api.github.com/users/lukedirtwalker/gists{/gist_id}","starred_url":"https://api.github.com/users/lukedirtwalker/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lukedirtwalker/subscriptions","organizations_url":"https://api.github.com/users/lukedirtwalker/orgs","repos_url":"https://api.github.com/users/lukedirtwalker/repos","events_url":"https://api.github.com/users/lukedirtwalker/events{/privacy}","received_events_url":"https://api.github.com/users/lukedirtwalker/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":5,"created_at":"2016-05-26T20:51:23Z","updated_at":"2016-05-29T11:50:34Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/dimitar-asenov/Envision/pulls/390","html_url":"https://github.com/dimitar-asenov/Envision/pull/390","diff_url":"https://github.com/dimitar-asenov/Envision/pull/390.diff","patch_url":"https://github.com/dimitar-asenov/Envision/pull/390.patch"},"body":"This allows us to add as many (virtual) breakpoints on a node as we\r\nwant. With this we can achieve nice things:\r\n-Since a breakpoint has an id we can remove the exact breakpoint while\r\nleaving other breakpoint on the same node intact.\r\n->This allows us to have multiple probes and tracked variables on a\r\nsingle node and theoretically also multiple user visible breakpoints.\r\n\r\nBy having a single listener method that can be registered to a\r\nbreakpoint id we can simplify our breakpoint handling method to finding\r\nall associated ids and calling the corresponding listener. This also\r\nmeans we can theoretically detach the track variable and probe\r\nimplementation from the debugger.\n<a href='#crh-start'></a><a href='#crh-data-%7B%22processed%22%3A%20%5B%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64887362%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64887517%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64888211%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64888401%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64888538%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64888689%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64888743%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64888928%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23issuecomment-222119566%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64903286%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64903357%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64903383%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23issuecomment-222145547%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23issuecomment-222148637%22%2C%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23issuecomment-222149246%22%5D%2C%20%22comments%22%3A%20%7B%22General%20Comment%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23issuecomment-222119566%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Alright%2C%20I%20looked%20at%20the%20code%2C%20but%20I%20must%20admit%2C%20I%27m%20very%20rusty%20when%20it%20comes%20to%20the%20debugging%20sub-system.%20I%20generally%20like%20the%20idea%20of%20breakpoint%20IDs%20and%20this%20patch.%20One%20thing%20I%20was%20wondering%3A%20does%20it%20make%20sense%20to%20somehow%20have%20another%20layer%20of%20abstraction%20that%20sits%20under%20the%20different%20types%20of%20breakpoints%20%28user%2C%20plots%2C%20tracked%20variables%2C%20probes%29.%20Would%20that%20allow%20us%20to%20unify%20some%20code%3F%22%2C%20%22created_at%22%3A%20%222016-05-27T11%3A02%3A06Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22I%27m%20not%20sure%20what%20you%20mean%20with%20another%20layer%20under%20the%20different%20types%20of%20breakpoint%2C%20would%20you%20mind%20to%20elaborate%3F%22%2C%20%22created_at%22%3A%20%222016-05-27T13%3A25%3A51Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3530769%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lukedirtwalker%22%7D%7D%2C%20%7B%22body%22%3A%20%22I%20meant%20for%20example%20a%20common%20base%20class%20for%20%60UserVisibleBreakpoint%60%2C%20%60ProbeVisualization%60%2C%20and%20%60TrackedVariable%60%2C%20since%20they%20all%20seem%20to%20be%20related%20to%20getting%20information%20via%20breakpoints.%20And%20perhaps%20just%20one%20common%20filed%20instead%20%60userBreakpoints_%60%2C%20%60probes_%60%2C%20and%20%60trackedVariables_%60%2C%20in%20JavaDebugger.%20I%20haven%27t%20really%20thought%20this%20through%2C%20I%20just%20observed%20that%20these%20three%20things%20get%20to%20often%20appear%20together%20and%20it%20might%20be%20cleaner%20to%20design%20a%20more%20abstract%20interface%20that%20handles%20all%20three%20at%20once%2C%20and%20just%20reimplement%20some%20virtual%20functions.%5Cr%5Cn%5Cr%5CnWhat%20do%20you%20think%3F%22%2C%20%22created_at%22%3A%20%222016-05-27T13%3A38%3A50Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22Regarding%20the%20type%2C%20I%20think%20I%27d%20prefer%20if%20for%20now%20you%20make%20it%20an%20int%20but%20also%20declare%20a%20custom%20name%20with%20%60using%60.%20This%20is%20good%20for%20two%20things%3A%5Cr%5Cn%20-%20a%20custom%20name%20is%20better%20recognizable%20throughout%20the%20code%20and%20also%20gives%20us%20the%20chance%20to%20change%20the%20underlying%20type%20in%20a%20single%20location%20if%20needed.%5Cr%5Cn%20-%20an%20int%20is%20the%20default%20integer%20type%20and%20we%20should%20always%20do%20that%20unless%20there%20is%20a%20good%20reason.%20using%20int64%20sends%20the%20message%20%5C%22the%20default%20%28int%29%20isn%27t%20good%20enough%20here%20and%20we%20need%20more%20bits%5C%22%2C%20but%20that%27s%20not%20the%20case%20it%20seems.%20If%20we%20ever%20do%20need%20to%20switch%20to%20an%20int64%2C%20we%20should%20comment%20on%20the%20%60using%60%20why%2064%20bits%20are%20needed.%22%2C%20%22created_at%22%3A%20%222016-05-27T13%3A41%3A23Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22General%20Comment%22%7D%2C%20%22Pull%20b6e8b6ce1d229aeac7ddfc2e83fa48014cf5d312%20OODebug/src/debugger/JavaDebugger.cpp%20443%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64888928%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Please%20don%27t%20commit%20this.%22%2C%20%22created_at%22%3A%20%222016-05-27T10%3A54%3A25Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20OODebug/src/debugger/JavaDebugger.cpp%3AL596-641%22%7D%2C%20%22Pull%20b6e8b6ce1d229aeac7ddfc2e83fa48014cf5d312%20OODebug/src/debugger/JavaDebugger.cpp%20139%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64888211%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22So%20ids%20-1%20and%200%20have%20a%20special%20meaning%3F%20We%20might%20want%20to%20document%20this%20at%20the%20point%20we%20define%20the%20breakpoint%20type.%22%2C%20%22created_at%22%3A%20%222016-05-27T10%3A46%3A39Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22We%20define%20them%20ourselves%20as%20you%20noted%20below%22%2C%20%22created_at%22%3A%20%222016-05-27T13%3A23%3A45Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3530769%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lukedirtwalker%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20OODebug/src/debugger/JavaDebugger.cpp%3AL245-266%22%7D%2C%20%22Pull%20b6e8b6ce1d229aeac7ddfc2e83fa48014cf5d312%20OODebug/src/debugger/JavaDebugger.cpp%20173%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64888401%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Please%20don%27t%20commit%20%60qDebug%60.%20If%20you%20think%20this%20is%20a%20generally%20useful%20message%20that%20we%20should%20display%20in%20the%20console%20during%20normal%20runs%20of%20the%20program%2C%20use%20the%20logging%20framework.%22%2C%20%22created_at%22%3A%20%222016-05-27T10%3A48%3A54Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20OODebug/src/debugger/JavaDebugger.cpp%3AL278-345%22%7D%2C%20%22Pull%20b6e8b6ce1d229aeac7ddfc2e83fa48014cf5d312%20OODebug/src/debugger/JavaDebugger.cpp%20267%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64888743%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Please%20remove%22%2C%20%22created_at%22%3A%20%222016-05-27T10%3A52%3A05Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20OODebug/src/debugger/JavaDebugger.cpp%3AL429-443%22%7D%2C%20%22Pull%20b6e8b6ce1d229aeac7ddfc2e83fa48014cf5d312%20OODebug/src/debugger/JavaDebugger.cpp%20184%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64888538%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22So%20it%20does%20seem%20like%20you%27re%20just%20creating%20the%20breakpoint%20ids%20yourself.%20Do%20we%20really%20need%20to%20be%20able%20to%20create%20int64%20amount%20of%20breakpoints%3F%22%2C%20%22created_at%22%3A%20%222016-05-27T10%3A50%3A11Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22no%20I%20think%20int%20would%20also%20be%20sufficient%22%2C%20%22created_at%22%3A%20%222016-05-27T13%3A24%3A02Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3530769%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lukedirtwalker%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20OODebug/src/debugger/JavaDebugger.cpp%3AL278-345%22%7D%2C%20%22Pull%20b6e8b6ce1d229aeac7ddfc2e83fa48014cf5d312%20OODebug/src/debugger/JavaDebugger.cpp%20221%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64888689%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22find%20%28not%20found%29%22%2C%20%22created_at%22%3A%20%222016-05-27T10%3A51%3A31Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20OODebug/src/debugger/JavaDebugger.cpp%3AL278-345%22%7D%2C%20%22Pull%20b6e8b6ce1d229aeac7ddfc2e83fa48014cf5d312%20OODebug/src/debugger/JavaDebugger.cpp%205%22%3A%20%7B%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov/Envision/pull/390%23discussion_r64887362%22%2C%20%22comments%22%3A%20%5B%7B%22body%22%3A%20%22Is%20there%20any%20specific%20reason%20why%20breakpoints%20are%20a%2064%20bit%20int%3F%20why%20not%20just%20%60int%60%3F%22%2C%20%22created_at%22%3A%20%222016-05-27T10%3A38%3A04Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22It%20might%20be%20nicer%20to%20define%20a%20custom%20breakpoint%20type%2C%20even%20if%20it%27s%20just%20%60using%20BreakpointId%20%3D%20int64%60.%22%2C%20%22created_at%22%3A%20%222016-05-27T10%3A39%3A38Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/1152976%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/dimitar-asenov%22%7D%7D%2C%20%7B%22body%22%3A%20%22No%20reason%20only%20if%20we%20want%20more%20than%20int_max%20%3A%29%22%2C%20%22created_at%22%3A%20%222016-05-27T13%3A23%3A12Z%22%2C%20%22user%22%3A%20%7B%22avatar_url%22%3A%20%22https%3A//avatars.githubusercontent.com/u/3530769%3Fv%3D3%22%2C%20%22html_url%22%3A%20%22https%3A//github.com/lukedirtwalker%22%7D%7D%5D%2C%20%22title%22%3A%20%22File%3A%20OODebug/src/debugger/JavaDebugger.cpp%3AL69-91%22%7D%7D%7D'></a>\n<a href='https://www.codereviewhub.com/'><img src='http://www.codereviewhub.com/site/github-bar.png' height=40></a>\n- [ ] <a href='#crh-comment-Pull b6e8b6ce1d229aeac7ddfc2e83fa48014cf5d312 OODebug/src/debugger/JavaDebugger.cpp 5'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/390#discussion_r64887362'>File: OODebug/src/debugger/JavaDebugger.cpp:L69-91</a></b>\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Is there any specific reason why breakpoints are a 64 bit int? why not just `int`?\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> It might be nicer to define a custom breakpoint type, even if it's just `using BreakpointId = int64`.\n- <a href='https://github.com/lukedirtwalker'><img border=0 src='https://avatars.githubusercontent.com/u/3530769?v=3' height=16 width=16'></a> No reason only if we want more than int_max :)\n- [ ] <a href='#crh-comment-Pull b6e8b6ce1d229aeac7ddfc2e83fa48014cf5d312 OODebug/src/debugger/JavaDebugger.cpp 139'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/390#discussion_r64888211'>File: OODebug/src/debugger/JavaDebugger.cpp:L245-266</a></b>\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> So ids -1 and 0 have a special meaning? We might want to document this at the point we define the breakpoint type.\n- <a href='https://github.com/lukedirtwalker'><img border=0 src='https://avatars.githubusercontent.com/u/3530769?v=3' height=16 width=16'></a> We define them ourselves as you noted below\n- [ ] <a href='#crh-comment-Pull b6e8b6ce1d229aeac7ddfc2e83fa48014cf5d312 OODebug/src/debugger/JavaDebugger.cpp 173'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/390#discussion_r64888401'>File: OODebug/src/debugger/JavaDebugger.cpp:L278-345</a></b>\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Please don't commit `qDebug`. If you think this is a generally useful message that we should display in the console during normal runs of the program, use the logging framework.\n- [ ] <a href='#crh-comment-Pull b6e8b6ce1d229aeac7ddfc2e83fa48014cf5d312 OODebug/src/debugger/JavaDebugger.cpp 184'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/390#discussion_r64888538'>File: OODebug/src/debugger/JavaDebugger.cpp:L278-345</a></b>\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> So it does seem like you're just creating the breakpoint ids yourself. Do we really need to be able to create int64 amount of breakpoints?\n- <a href='https://github.com/lukedirtwalker'><img border=0 src='https://avatars.githubusercontent.com/u/3530769?v=3' height=16 width=16'></a> no I think int would also be sufficient\n- [ ] <a href='#crh-comment-Pull b6e8b6ce1d229aeac7ddfc2e83fa48014cf5d312 OODebug/src/debugger/JavaDebugger.cpp 221'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/390#discussion_r64888689'>File: OODebug/src/debugger/JavaDebugger.cpp:L278-345</a></b>\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> find (not found)\n- [ ] <a href='#crh-comment-Pull b6e8b6ce1d229aeac7ddfc2e83fa48014cf5d312 OODebug/src/debugger/JavaDebugger.cpp 267'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/390#discussion_r64888743'>File: OODebug/src/debugger/JavaDebugger.cpp:L429-443</a></b>\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Please remove\n- [ ] <a href='#crh-comment-Pull b6e8b6ce1d229aeac7ddfc2e83fa48014cf5d312 OODebug/src/debugger/JavaDebugger.cpp 443'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/390#discussion_r64888928'>File: OODebug/src/debugger/JavaDebugger.cpp:L596-641</a></b>\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Please don't commit this.\n- [ ] <a href='#crh-comment-General Comment'></a> <img src='http://www.codereviewhub.com/site/github-remaining.png' height=16 width=60>&nbsp;<b><a href='https://github.com/dimitar-asenov/Envision/pull/390#issuecomment-222119566'>General Comment</a></b>\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Alright, I looked at the code, but I must admit, I'm very rusty when it comes to the debugging sub-system. I generally like the idea of breakpoint IDs and this patch. One thing I was wondering: does it make sense to somehow have another layer of abstraction that sits under the different types of breakpoints (user, plots, tracked variables, probes). Would that allow us to unify some code?\n- <a href='https://github.com/lukedirtwalker'><img border=0 src='https://avatars.githubusercontent.com/u/3530769?v=3' height=16 width=16'></a> I'm not sure what you mean with another layer under the different types of breakpoint, would you mind to elaborate?\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> I meant for example a common base class for `UserVisibleBreakpoint`, `ProbeVisualization`, and `TrackedVariable`, since they all seem to be related to getting information via breakpoints. And perhaps just one common filed instead `userBreakpoints_`, `probes_`, and `trackedVariables_`, in JavaDebugger. I haven't really thought this through, I just observed that these three things get to often appear together and it might be cleaner to design a more abstract interface that handles all three at once, and just reimplement some virtual functions.\nWhat do you think?\n- <a href='https://github.com/dimitar-asenov'><img border=0 src='https://avatars.githubusercontent.com/u/1152976?v=3' height=16 width=16'></a> Regarding the type, I think I'd prefer if for now you make it an int but also declare a custom name with `using`. This is good for two things:\n- a custom name is better recognizable throughout the code and also gives us the chance to change the underlying type in a single location if needed.\n- an int is the default integer type and we should always do that unless there is a good reason. using int64 sends the message \"the default (int) isn't good enough here and we need more bits\", but that's not the case it seems. If we ever do need to switch to an int64, we should comment on the `using` why 64 bits are needed.\n\n\n<a href='https://www.codereviewhub.com/dimitar-asenov/Envision/pull/390?mark_as_completed=1'><img src='http://www.codereviewhub.com/site/github-mark-as-completed.png' height=26></a>&nbsp;<a href='https://www.codereviewhub.com/dimitar-asenov/Envision/pull/390?approve=1'><img src='http://www.codereviewhub.com/site/github-approve.png' height=26></a>&nbsp;<a href='https://github.com/dimitar-asenov/Envision/pull/390'><img src='http://www.codereviewhub.com/site/github-refresh.png' height=26></a>\n<a href='#crh-end'></a>"},"comment":{"url":"https://api.github.com/repos/dimitar-asenov/Envision/issues/comments/222356579","html_url":"https://github.com/dimitar-asenov/Envision/pull/390#issuecomment-222356579","issue_url":"https://api.github.com/repos/dimitar-asenov/Envision/issues/390","id":222356579,"user":{"login":"lukedirtwalker","id":3530769,"avatar_url":"https://avatars.githubusercontent.com/u/3530769?v=3","gravatar_id":"","url":"https://api.github.com/users/lukedirtwalker","html_url":"https://github.com/lukedirtwalker","followers_url":"https://api.github.com/users/lukedirtwalker/followers","following_url":"https://api.github.com/users/lukedirtwalker/following{/other_user}","gists_url":"https://api.github.com/users/lukedirtwalker/gists{/gist_id}","starred_url":"https://api.github.com/users/lukedirtwalker/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/lukedirtwalker/subscriptions","organizations_url":"https://api.github.com/users/lukedirtwalker/orgs","repos_url":"https://api.github.com/users/lukedirtwalker/repos","events_url":"https://api.github.com/users/lukedirtwalker/events{/privacy}","received_events_url":"https://api.github.com/users/lukedirtwalker/received_events","type":"User","site_admin":false},"created_at":"2016-05-29T11:50:34Z","updated_at":"2016-05-29T11:50:34Z","body":"So I added to more commits to address your comments.\r\n\r\nAbout the idea of merging probes, user breakpoints and tracked variables into a common interface: I'm not so sure I like it.\r\n\r\nI don't think they appear together:\r\n`trackedVariables_` is only used in `JavaDebugger::trackVariable()` and `probes_` is only used in `JavaDebugger::probe()`, `userBreakpoints` is used in some more places (maybe wrongly so).\r\n\r\nI'd rather externalize variable tracking and probes and maybe even user breakpoints into separate classes which only use add- and removeBreakpoint methods and the `BreakpointListener` from the `JavaDebugger`. In my opinion this is the only thing the three things should share from the debuggers perspective. They also share visualization mechanisms, but I think all three are different concepts.\r\n\r\nLet me know what you think."}}
{"actor":{"display_login":"lukedirtwalker"}}