Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fixes #11474 #11751

Merged
merged 1 commit into from
Jul 17, 2019
Merged

fixes #11474 #11751

merged 1 commit into from
Jul 17, 2019

Conversation

Araq
Copy link
Member

@Araq Araq commented Jul 16, 2019

No description provided.

@Araq Araq force-pushed the araq-fixes-11474 branch from 1550503 to 15d2a92 Compare July 17, 2019 13:20
@Araq Araq merged commit 326860e into devel Jul 17, 2019
@Araq Araq deleted the araq-fixes-11474 branch July 17, 2019 13:21
kaushalmodi pushed a commit to kaushalmodi/Nim that referenced this pull request Jul 17, 2019
@timotheecour
Copy link
Member

=> #11773

@krux02
Copy link
Contributor

krux02 commented Jul 18, 2019

I have to support the criticism of @zah here.

  • no documentation
  • no specification
  • no tests for error messages
  • no review process (did we ever even consider if using the [] operator here could introduce ambiguities in the Nim grammar?)

Introducing new features into Nim like this can really Hurt the language. Please revert this commit and make a proper RFC for it with a specification of the behavior. And then the PR must contain tests for that ensure the misuses of unref will trigger proper error messages, instead of doing undefined behavior.

@dom96
Copy link
Contributor

dom96 commented Jul 18, 2019

No need to revert it, but discussion should be had, I agree with @krux02 and @zah on that.

@Araq
Copy link
Member Author

Araq commented Jul 19, 2019

did we ever even consider if using the [] operator here could introduce ambiguities in the Nim grammar?

How could it? The parser was left untouched. But I completely agree with all your other points.

@krux02
Copy link
Contributor

krux02 commented Jul 22, 2019

@Araq since this PR got removed again, I think it is not that relevant anymore but

How could it? The parser was left untouched. But I completely agree with all your other points.

An nkBracketExpr on a type is usually a generic invocation. That is the ambiguity I am talking about. And my point is not if it is ambiguous, but that we should at least have a review process to see if there could be an ambiguity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants