{"action":"created","issue":{"url":"https://api.github.com/repos/fish-shell/fish-shell/issues/3276","repository_url":"https://api.github.com/repos/fish-shell/fish-shell","labels_url":"https://api.github.com/repos/fish-shell/fish-shell/issues/3276/labels{/name}","comments_url":"https://api.github.com/repos/fish-shell/fish-shell/issues/3276/comments","events_url":"https://api.github.com/repos/fish-shell/fish-shell/issues/3276/events","html_url":"https://github.com/fish-shell/fish-shell/issues/3276","id":168551294,"number":3276,"title":"should fish continue to use `assert()`?","user":{"login":"krader1961","id":7545917,"avatar_url":"https://avatars.githubusercontent.com/u/7545917?v=3","gravatar_id":"","url":"https://api.github.com/users/krader1961","html_url":"https://github.com/krader1961","followers_url":"https://api.github.com/users/krader1961/followers","following_url":"https://api.github.com/users/krader1961/following{/other_user}","gists_url":"https://api.github.com/users/krader1961/gists{/gist_id}","starred_url":"https://api.github.com/users/krader1961/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/krader1961/subscriptions","organizations_url":"https://api.github.com/users/krader1961/orgs","repos_url":"https://api.github.com/users/krader1961/repos","events_url":"https://api.github.com/users/krader1961/events{/privacy}","received_events_url":"https://api.github.com/users/krader1961/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/fish-shell/fish-shell/labels/cleanup","name":"cleanup","color":"666666"},{"url":"https://api.github.com/repos/fish-shell/fish-shell/labels/enhancement","name":"enhancement","color":"84b6eb"}],"state":"open","locked":false,"assignee":{"login":"krader1961","id":7545917,"avatar_url":"https://avatars.githubusercontent.com/u/7545917?v=3","gravatar_id":"","url":"https://api.github.com/users/krader1961","html_url":"https://github.com/krader1961","followers_url":"https://api.github.com/users/krader1961/followers","following_url":"https://api.github.com/users/krader1961/following{/other_user}","gists_url":"https://api.github.com/users/krader1961/gists{/gist_id}","starred_url":"https://api.github.com/users/krader1961/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/krader1961/subscriptions","organizations_url":"https://api.github.com/users/krader1961/orgs","repos_url":"https://api.github.com/users/krader1961/repos","events_url":"https://api.github.com/users/krader1961/events{/privacy}","received_events_url":"https://api.github.com/users/krader1961/received_events","type":"User","site_admin":false},"assignees":[{"login":"krader1961","id":7545917,"avatar_url":"https://avatars.githubusercontent.com/u/7545917?v=3","gravatar_id":"","url":"https://api.github.com/users/krader1961","html_url":"https://github.com/krader1961","followers_url":"https://api.github.com/users/krader1961/followers","following_url":"https://api.github.com/users/krader1961/following{/other_user}","gists_url":"https://api.github.com/users/krader1961/gists{/gist_id}","starred_url":"https://api.github.com/users/krader1961/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/krader1961/subscriptions","organizations_url":"https://api.github.com/users/krader1961/orgs","repos_url":"https://api.github.com/users/krader1961/repos","events_url":"https://api.github.com/users/krader1961/events{/privacy}","received_events_url":"https://api.github.com/users/krader1961/received_events","type":"User","site_admin":false}],"milestone":{"url":"https://api.github.com/repos/fish-shell/fish-shell/milestones/4","html_url":"https://github.com/fish-shell/fish-shell/milestones/fish-future","labels_url":"https://api.github.com/repos/fish-shell/fish-shell/milestones/4/labels","id":130257,"number":4,"title":"fish-future","description":"These are issues that are not targeted for an upcoming minor or major release. After a release, we will re-examine these issues and pull any in that we think are good targets; that is retarget them to the next minor (or major) release.\r\n\r\nIf you can provide a fix for an issue in this milestone, we'll generally accept it for the next release even if the issue is not targeted for the release.","creator":{"login":"ridiculousfish","id":920838,"avatar_url":"https://avatars.githubusercontent.com/u/920838?v=3","gravatar_id":"","url":"https://api.github.com/users/ridiculousfish","html_url":"https://github.com/ridiculousfish","followers_url":"https://api.github.com/users/ridiculousfish/followers","following_url":"https://api.github.com/users/ridiculousfish/following{/other_user}","gists_url":"https://api.github.com/users/ridiculousfish/gists{/gist_id}","starred_url":"https://api.github.com/users/ridiculousfish/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ridiculousfish/subscriptions","organizations_url":"https://api.github.com/users/ridiculousfish/orgs","repos_url":"https://api.github.com/users/ridiculousfish/repos","events_url":"https://api.github.com/users/ridiculousfish/events{/privacy}","received_events_url":"https://api.github.com/users/ridiculousfish/received_events","type":"User","site_admin":false},"open_issues":381,"closed_issues":2,"state":"open","created_at":"2012-06-11T02:25:13Z","updated_at":"2016-08-14T02:14:11Z","due_on":null,"closed_at":null},"comments":18,"created_at":"2016-08-01T01:43:05Z","updated_at":"2016-08-16T03:10:47Z","closed_at":null,"body":"A recent change was committed to add a `default: assert(0 && \"msg\");` clause to `switch (condition) {` blocks  where `condition` is expected to be a known enum value. That change is fine. But it got me thinking about all the ways that fish currently deals with \"can't happen\" errors. There's the `assert(0 && \"msg\")` idiom. There are direct calls to `abort()`. There are calls to `DIE(\"msg\")` or `DIE_MEM()`. There is `sanity_lose()`. Despite making numerous changes over the past nine months I'm still uncertain about which of the aforementioned options to use.\r\n\r\nThe `DIE()` macro has a lot to recommend it. But there are two aspects that could be improved. First, it should be possible to pass it a parameterized message and arguments to be interpolated into the message. Second, the `read_ignore()` call in `FATAL_EXIT()` (called by `DIE()`) should only be done if the shell is interactive. And if it is interactive there should be a prompt before the read to tell the user to press any key to terminate the shell after they've copied the diagnostic data.\r\n\r\nI'm inclined to make those improvements to `DIE()` and replace all `assert(0 && \"msg\")` and `abort()` calls with equivalent `DIE()` calls. As part of this work we should document in *CONTRIBUTING.md* the alternatives and when they should be used."},"comment":{"url":"https://api.github.com/repos/fish-shell/fish-shell/issues/comments/239990254","html_url":"https://github.com/fish-shell/fish-shell/issues/3276#issuecomment-239990254","issue_url":"https://api.github.com/repos/fish-shell/fish-shell/issues/3276","id":239990254,"user":{"login":"krader1961","id":7545917,"avatar_url":"https://avatars.githubusercontent.com/u/7545917?v=3","gravatar_id":"","url":"https://api.github.com/users/krader1961","html_url":"https://github.com/krader1961","followers_url":"https://api.github.com/users/krader1961/followers","following_url":"https://api.github.com/users/krader1961/following{/other_user}","gists_url":"https://api.github.com/users/krader1961/gists{/gist_id}","starred_url":"https://api.github.com/users/krader1961/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/krader1961/subscriptions","organizations_url":"https://api.github.com/users/krader1961/orgs","repos_url":"https://api.github.com/users/krader1961/repos","events_url":"https://api.github.com/users/krader1961/events{/privacy}","received_events_url":"https://api.github.com/users/krader1961/received_events","type":"User","site_admin":false},"created_at":"2016-08-16T03:10:47Z","updated_at":"2016-08-16T03:10:47Z","body":"> I think all the occurrences of DIE() I can find would be better nixed one way or the other. I think assert() is the thing to use for runtime assertions.\r\n\r\nYour opinion is noted. But what exactly do you think will happen when one of those asserts is triggered on a user's system that does not have core dumps enabled or who cannot even give us a backtrace because they can't even follow the most basic instructions for running gdb (which is most users)? We'll get a generic error that will probably be insufficient to reach root cause or even suggest avenues for subsequent analysis. Granted, a more detailed error that includes the values of run-time variables as well as a call stack is also no guarantee we can determine root cause of the failure. But it does increase the likelihood we can do so.\r\n\r\nI'm not totally opposed to replacing all `DIE()` calls with `assert()` or `abort()`. The whole point of this issue is to decide on a consistent mechanism for handling unexpected \"can't happen\" failures. If the consensus is to eliminate `DIE()` with the more generic mechanisms I can live with that decision."}}
{"actor":{"display_login":"krader1961"}}