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

fix #6472 Typed template arguments are substituted directly if they a… #20399

Closed
wants to merge 2 commits into from

Conversation

bung87
Copy link
Collaborator

@bung87 bung87 commented Sep 20, 2022

…re compatible with parameter type

fix #6472

after CI fails it seems need fix exists code for this.

@Varriount Varriount requested a review from Araq September 20, 2022 19:16
@Varriount Varriount added the Requires Araq To Merge PR should only be merged by Araq label Sep 20, 2022
@Araq
Copy link
Member

Araq commented Sep 22, 2022

I don't understand this fix.

@bung87
Copy link
Collaborator Author

bung87 commented Sep 22, 2022

have you check the issue? before this arguments will pass template's arguments type check, however when construct array will throw type mismatch. which means it passed as int.

@bung87
Copy link
Collaborator Author

bung87 commented Sep 22, 2022

also mentioned here #18667 (comment)

@metagn
Copy link
Collaborator

metagn commented Sep 22, 2022

While this looks like a hack at first glance, it's a special cased version of doing argNode = semExpr(argNode, expectedType = paramType) for integers. The issue is a consequence of fitNode not being called on template parameters.

Maybe this is too special cased, and I'm not sure if it works with varargs. Since this breaks code, it could be made an experimental feature, but I'm not sure who would opt in to it.

@bung87
Copy link
Collaborator Author

bung87 commented Sep 23, 2022

While this looks like a hack at first glance, it's a special cased version of doing argNode = semExpr(argNode, expectedType = paramType) for integers. The issue is a consequence of fitNode not being called on template parameters.

Maybe this is too special cased, and I'm not sure if it works with varargs. Since this breaks code, it could be made an experimental feature, but I'm not sure who would opt in to it.

I dont think it's "special cased" or "made an experimental feature", it break packages code that depends on odd behavior, that can be fixed in user code, the problem is obvious to me, it doesn't respect concret param type user specified. I think whether it fixed like this or in other way.

@bung87
Copy link
Collaborator Author

bung87 commented Sep 30, 2022

template arguments not check right now, it may do check someday, at that time no need handle this case. so I'll close now.

@bung87 bung87 closed this Sep 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Requires Araq To Merge PR should only be merged by Araq
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typed template arguments are substituted directly if they are compatible with parameter type
4 participants