-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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 #15958 #15970
fix #15958 #15970
Conversation
compiler/ccgtypes.nim
Outdated
@@ -282,7 +282,7 @@ proc ccgIntroducedPtr(conf: ConfigRef; s: PSym, retType: PType): bool = | |||
result = false | |||
# first parameter and return type is 'lent T'? --> use pass by pointer | |||
if s.position == 0 and retType != nil and retType.kind == tyLent: | |||
result = pt.kind != tyVar | |||
result = pt.kind notin {tyVar, tyArray} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But C arrays are pointers already, why would the C code generator introduce another indirection in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix actually removes double pointer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then what about tyOpenArray, tyVarargs
(and maybe tySequence
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right tyOpenArray, tyVarargs also need this fix. Seq is fine.
Added tests to cover tyOpenArray, tyVarargs to PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, big sets are mapped to C arrays... :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they are and they need the fix. Added big sets as well :)
ready for review |
* followup nim-lang#15970 strenghten regression tests * _
* fix nim-lang#15958 * also cover openArray and VarArgs * more tests * cover even more types * cover even more types * Trigger build * Trigger build * cover sets passed as arrays
* followup nim-lang#15970 strenghten regression tests * _
* fix nim-lang#15958 * also cover openArray and VarArgs * more tests * cover even more types * cover even more types * Trigger build * Trigger build * cover sets passed as arrays
* followup nim-lang#15970 strenghten regression tests * _
fix #15958