{"action":"created","issue":{"url":"https://api.github.com/repos/bwapi/bwapi/issues/575","labels_url":"https://api.github.com/repos/bwapi/bwapi/issues/575/labels{/name}","comments_url":"https://api.github.com/repos/bwapi/bwapi/issues/575/comments","events_url":"https://api.github.com/repos/bwapi/bwapi/issues/575/events","html_url":"https://github.com/bwapi/bwapi/issues/575","id":69646980,"number":575,"title":"Game.getRegionAt() returns Region for invalid positions causing crash","user":{"login":"ratiotile","id":7064626,"avatar_url":"https://avatars.githubusercontent.com/u/7064626?v=3","gravatar_id":"","url":"https://api.github.com/users/ratiotile","html_url":"https://github.com/ratiotile","followers_url":"https://api.github.com/users/ratiotile/followers","following_url":"https://api.github.com/users/ratiotile/following{/other_user}","gists_url":"https://api.github.com/users/ratiotile/gists{/gist_id}","starred_url":"https://api.github.com/users/ratiotile/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ratiotile/subscriptions","organizations_url":"https://api.github.com/users/ratiotile/orgs","repos_url":"https://api.github.com/users/ratiotile/repos","events_url":"https://api.github.com/users/ratiotile/events{/privacy}","received_events_url":"https://api.github.com/users/ratiotile/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/bwapi/bwapi/labels/bug","name":"bug","color":"fc2929"},{"url":"https://api.github.com/repos/bwapi/bwapi/labels/Priority-Critical","name":"Priority-Critical","color":"FFFFFF"},{"url":"https://api.github.com/repos/bwapi/bwapi/labels/question","name":"question","color":"cc317c"}],"state":"open","locked":false,"assignee":{"login":"heinermann","id":1966366,"avatar_url":"https://avatars.githubusercontent.com/u/1966366?v=3","gravatar_id":"","url":"https://api.github.com/users/heinermann","html_url":"https://github.com/heinermann","followers_url":"https://api.github.com/users/heinermann/followers","following_url":"https://api.github.com/users/heinermann/following{/other_user}","gists_url":"https://api.github.com/users/heinermann/gists{/gist_id}","starred_url":"https://api.github.com/users/heinermann/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/heinermann/subscriptions","organizations_url":"https://api.github.com/users/heinermann/orgs","repos_url":"https://api.github.com/users/heinermann/repos","events_url":"https://api.github.com/users/heinermann/events{/privacy}","received_events_url":"https://api.github.com/users/heinermann/received_events","type":"User","site_admin":false},"milestone":{"url":"https://api.github.com/repos/bwapi/bwapi/milestones/2","html_url":"https://github.com/bwapi/bwapi/milestones/4.1.1","labels_url":"https://api.github.com/repos/bwapi/bwapi/milestones/2/labels","id":959632,"number":2,"title":"4.1.1","description":"This milestone should contain bug fixes and general improvements for 4.1.0.\r\n\r\n","creator":{"login":"heinermann","id":1966366,"avatar_url":"https://avatars.githubusercontent.com/u/1966366?v=3","gravatar_id":"","url":"https://api.github.com/users/heinermann","html_url":"https://github.com/heinermann","followers_url":"https://api.github.com/users/heinermann/followers","following_url":"https://api.github.com/users/heinermann/following{/other_user}","gists_url":"https://api.github.com/users/heinermann/gists{/gist_id}","starred_url":"https://api.github.com/users/heinermann/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/heinermann/subscriptions","organizations_url":"https://api.github.com/users/heinermann/orgs","repos_url":"https://api.github.com/users/heinermann/repos","events_url":"https://api.github.com/users/heinermann/events{/privacy}","received_events_url":"https://api.github.com/users/heinermann/received_events","type":"User","site_admin":false},"open_issues":29,"closed_issues":10,"state":"open","created_at":"2015-01-31T19:18:36Z","updated_at":"2015-04-22T12:50:40Z","due_on":"2015-04-30T04:00:00Z","closed_at":null},"comments":7,"created_at":"2015-04-20T18:12:29Z","updated_at":"2015-04-24T02:06:17Z","closed_at":null,"body":"If you aren't careful and accidentally get a region from an invalid off-the-map Position, Broodwar will go ahead and return a Region, but it will crash the bot if you access any method on the Region.\r\n\r\nThe following code crashes the bot.\r\n\r\n\t  Position pos(-1, -1);\r\n\t  BWAPI::Region test_region(Broodwar->getRegionAt(pos));  // everything still OK\r\n\t  Broodwar << \"Region ID is \" << test_region->getID() << \" accessible? \" << test_region->isAccessible() << std::endl;\r\n\r\nWindows 7x64, BWAPI v4.1.0 via ExampleClient, release mode\r\n![image](https://cloud.githubusercontent.com/assets/7064626/7237230/28142b6c-e768-11e4-8fa9-b39afeedf84d.png)\r\n"},"comment":{"url":"https://api.github.com/repos/bwapi/bwapi/issues/comments/95774028","html_url":"https://github.com/bwapi/bwapi/issues/575#issuecomment-95774028","issue_url":"https://api.github.com/repos/bwapi/bwapi/issues/575","id":95774028,"user":{"login":"ratiotile","id":7064626,"avatar_url":"https://avatars.githubusercontent.com/u/7064626?v=3","gravatar_id":"","url":"https://api.github.com/users/ratiotile","html_url":"https://github.com/ratiotile","followers_url":"https://api.github.com/users/ratiotile/followers","following_url":"https://api.github.com/users/ratiotile/following{/other_user}","gists_url":"https://api.github.com/users/ratiotile/gists{/gist_id}","starred_url":"https://api.github.com/users/ratiotile/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ratiotile/subscriptions","organizations_url":"https://api.github.com/users/ratiotile/orgs","repos_url":"https://api.github.com/users/ratiotile/repos","events_url":"https://api.github.com/users/ratiotile/events{/privacy}","received_events_url":"https://api.github.com/users/ratiotile/received_events","type":"User","site_admin":false},"created_at":"2015-04-24T02:06:17Z","updated_at":"2015-04-24T02:06:17Z","body":"I see your point in just leaving the return as nullptr. A Regions::Invalid dummy would also work, and goes along with Positions::Invalid, though you would need to make one for Unit too. A change to throw exceptions would be incompatible with existing codebases.\r\n\r\nWhen I wrote a c++ bot I never had the problem dealing with nullptr: as you said, I would just check the pointer each time. The issue was in producing a Python wrapper that could deal with the null pointer. It can't crash to desktop without a way to catch/check it. I ruled out pos.makeValid() since it is too easy to forget it, and there is no indication where in the code the crash is coming from. What I have done instead is to throw an exception in the wrapper as soon as the nullptr is returned. An example:\r\n\r\n    ##Python\r\n    for unit in units:\r\n        try:\r\n            target = unit.getTarget()\r\n            Broodwar.drawLineMap(target.getPosition(), unit.getPosition(), cybw.Colors.White)\r\n        except cybw.NullPointerException:\r\n            continue\r\n\r\nA strong reason to use exceptions instead is that it would help to people write more stable bots, once they learned how to deal with them. Why should functions ever return nullptr? There is no reason to keep around nullptrs. You are going to check it immediately and then discard it if it is null. Best part is, if you forget to use try/catch, your code blows up immediately vs. forgetting to check `== nullptr` and having a null Region that might turn up later and cause a crash. The two ways basically take up the same amount of code, too. However, null-checking might take more code if you check in each function (defensive programming).\r\n\r\n10 lines:\r\n\r\n    Position pos(-1, -1);\r\n    BWAPI::Region test_region(Broodwar->getRegionAt(pos));\r\n    if (test_region != nullptr) \r\n    {\r\n        Broodwar << \"Region ID is \" << test_region->getID();\r\n    }\r\n    else\r\n    {\r\n        Broodwar << \"invalid Region\";\r\n    }\r\n\r\nstill 10 lines with exceptions:\r\n\r\n    Position pos(-1, -1);\r\n    try\r\n    {\r\n        BWAPI::Region test_region(Broodwar->getRegionAt(pos));\r\n        Broodwar << \"Region ID is \" << test_region->getID();\r\n    }\r\n    catch(exception& e)\r\n    {\r\n        Broodwar << \"invalid Region\";\r\n    }\r\n\r\nThink about it.\r\n\r\nIn any case, BWAPI probably shouldn't change this right now. I bet there are far more pressing issues to resolve."}}