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

Replace enum fields idents with syms #14048

Merged
merged 4 commits into from
Apr 21, 2020
Merged

Replace enum fields idents with syms #14048

merged 4 commits into from
Apr 21, 2020

Conversation

cooldome
Copy link
Member

During review of #14030 and #14046 I found there is problem with getImpl() for enum types.
Fields are idents and not syms. This makes hard to write typed macros. You have to generate
EnumType.X everywhere instead of X. But it doesn't work for bool enum type. bool.true is not valid.

@@ -62,6 +62,7 @@ proc semEnum(c: PContext, n: PNode, prev: PType): PType =
counter, x: BiggestInt
e: PSym
base: PType
identToReplace: PNode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer identToReplace: ptr PNode.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@Araq
Copy link
Member

Araq commented Apr 21, 2020

This is a breaking change and needs a migration mechanism.

@cooldome
Copy link
Member Author

old behaviour can now be restored with --oldast switch.

Ready for review

@Araq Araq merged commit 5db0bb7 into devel Apr 21, 2020
@Araq Araq deleted the enumField_as_sym branch April 21, 2020 18:12
@timotheecour
Copy link
Member

good PR but --oldast is really not meaningful at all (eg: how old?) and already had a different meaning, making it hard/impossible for users to migrate their code from right before your PR to right after your PR, since the --oldast would imply other unrelated changes.

The correct way to handle breaking changes is simple and involves both:

  • make this controlled by --legacy:enumFieldsIdents (or -d:nimLegacyEnumFieldsIdents if feeling lazy)
  • make --useVersion:1.0 (and --useVersion:1.2) define nimLegacyEnumFieldsIdents

For testing legacy, we avoid combinatorial explosion by only testing releases eg --useVersion:1.0 and --useVersion:1.2, and individual breaking changes are best effort.

@Araq
Copy link
Member

Araq commented Apr 21, 2020

@timotheecour I'm aware and will change it but was tired of the review ping-pong and so took the shortcut and merged it already.

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.

3 participants