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

meta/codegen.d: Fix getSymbols for dlang/dmd#11320 #2458

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ntrel
Copy link

@ntrel ntrel commented Jul 10, 2020

This fixes #2455. Instead of changing the assert I just made getSymbols continue dropping qualifiers, because that will work with both existing and future dmd.

See dlang/dmd#11320.

@Geod24
Copy link
Contributor

Geod24 commented Jul 10, 2020

A comment would be nice

@ntrel
Copy link
Author

ntrel commented Jul 10, 2020

@Geod24 I've edited the description and added links, hope that's ok

@@ -78,7 +78,7 @@ template getSymbols(T)
alias Implementation = TypeTuple!();
}

alias getSymbols = NoDuplicates!(Implementation!T);
alias getSymbols = NoDuplicates!(staticMap!(Unqual, Implementation!T));
Copy link
Contributor

Choose a reason for hiding this comment

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

This has compile-time performance implications.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a breaking change? Wouldn't the original say AliasSeq!(int, const(int)) for int[const(int)]? alias parameters seem to allow const(int) to go through, whereas Unqual!(const(int)) turns into int

Copy link
Author

Choose a reason for hiding this comment

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

@schveiguy Oops you're right

Copy link
Author

Choose a reason for hiding this comment

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

Actually I think basic types are ignored anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, you are right, I didn't realize that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this can you apply Unqual at the leaf instantiations? That would prevent the need for static map.

@ntrel ntrel changed the title Fix buildkite/dmd for https://github.com/dlang/dmd/pull/11320 meta/codegen.d: Fix getSymbols for dlang/dmd#11320 Jul 10, 2020
@schveiguy
Copy link
Contributor

looking at the usage (figure out which imports need to happen to include these symbols), I think this change is correct in terms of what this thing is meant to do. I would suggest a document comment that clarifies that attributes will be stripped from types.

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.

Heads up: dmd fix for alias parameter missing qualifier breaks unittest
4 participants