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.

Sign up for GitHub

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

Jump to bottom

fixes #4799, varargs now can accept polymorphic types #7550

Merged
merged 4 commits into from
Jun 6, 2018
Merged

fixes #4799, varargs now can accept polymorphic types #7550

merged 4 commits into from
Jun 6, 2018

Conversation

jangko
Copy link
Contributor

@jangko jangko commented Apr 9, 2018

No description provided.

Araq
Araq reviewed Apr 9, 2018
View reviewed changes
compiler/ccgexprs.nim Outdated
@@ -347,7 +347,8 @@ proc genAssignment(p: BProc, dest, src: TLoc, flags: TAssignmentFlags) =
else:
useStringh(p.module)
linefmt(p, cpsStmts,
"memcpy((void*)$1, (NIM_CONST void*)$2, sizeof($1[0])*$1Len_0);$n",
#"memcpy((void*)$1, (NIM_CONST void*)$2, sizeof($1[0])*$1Len_0);$n",
"$1 = $2;$n",
Copy link
Member

Choose a reason for hiding this comment

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

How is this correct? C arrays have no assignment operator.

Copy link
Contributor Author

@jangko jangko Apr 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

  • because of bug varags broken with polymorphic types #4799, this 'else' branch never executed anymore.
  • all other Nim types of varargs are not handled through this 'else' branch. they are handled somewhere else. Only ref object are handled by this 'else' branch. Ordinary object and ptr object are not handled here.
  • I also tried various openArray, they also never handled by this 'else' branch
  • the above code also not a C array assignment, but plain pointer assignment.
  • the transform part already transform the AST into a series of single pointer assignment(s), not a sequence of pointers assignment anymore.
  • because of this reasons, the memcpy no longer needed, at least we haven't encountered other use case of this else branch besides simple pointer assignment.

"Len_0" also not used anymore, bad codegen here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

perhaps a clear annotation/comment for this else branch sufficient, until another bug comes up?

@Araq
Copy link
Member

Araq commented Apr 12, 2018

Does it work with an explicit [] array construction too? Please add that to the test. Then it should be fine.

@jangko
Copy link
Contributor Author

jangko commented Apr 12, 2018 •
edited by data-man
Loading

I found three more issues with this case:

  • sigmatch bug: array construction works with ref object and ptr object, not work with generic object
  • sigmatch bug: ordinary object will crash/garbage at runtime. need to locate the problem. the generated code not using memcpy, only a deep nested curly braces and address operator.
  • sigmatch bug: generic ordinary object descended from generic ref object can fool sigmatch without error

remind myself:

  • fix sigmatch bug for ordinary object
  • fix sigmatch for array of generic objects
  • fix sigmatch for ordinary object descended from generic object -> make it smarter, foolproof
  • add test for ordinary object descended from generic ref object -> produce error
  • add test for ordinary object descended from generic ref object of regular proc/no varargs -> produce error
  • add test for ordinary object descended from ref object -> produce error
  • test proc scope too, not only global scope
  • test generic ref object
  • test generic ptr object
  • test array [ ] generic
  • explicit array construction [ ] test

Araq
Araq reviewed Apr 21, 2018
View reviewed changes
compiler/sigmatch.nim Outdated
@@ -2000,6 +2000,13 @@ proc paramTypesMatchAux(m: var TCandidate, f, a: PType,
m.baseTypeMatch = true
else:
result = userConvMatch(c, m, base(f), a, arg)
# bug #4799, varargs accepting subtype relation object
if result.isNil and r == isSubtype:
Copy link
Member

Choose a reason for hiding this comment

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

The check for r == isSubtype needs to be moved above userConvMatch, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the tip, I will take a look, after #7600 and #7601

@jangko
Copy link
Contributor Author

jangko commented Apr 28, 2018 •
edited
Loading

#7600 and #7601 already merged, now I can focus on this one. and this is the new todo list:

  • fix sigmatch bug for varargs accepting ordinary object descended from ordinary object
  • fix sigmatch bug for varargs accepting array of generic objects
  • add test for varargs accepting ordinary object descended from generic ref object -> should error
  • add test for varargs accepting ordinary object descended from ref object -> should error
  • add test for varargs accepting ordinary object descended from generic ptr object -> should error
  • add test for varargs accepting ordinary object descended from ptr object -> should error
  • test varargs accepting generic object
  • test varargs accepting generic ref object
  • test varargs accepting generic ptr object
  • test varargs accepting object
  • test varargs accepting ref object
  • test varargs accepting ptr object
  • test varargs accepting array [] of generic objects
  • test varargs accepting array [] of ref generic objects
  • test varargs accepting array [] of ptr generic objects
  • test varargs accepting array [] of objects
  • test varargs accepting array [] of ref objects
  • test varargs accepting array [] of ptr objects
  • test varargs accepting specialized generic
  • test varargs accepting array [] of specialized generic
  • test varargs accepting partial specialized generic
  • test varargs accepting array [] of partial specialized generic
  • test calling varargs inside proc scope too, not only global scope

@jangko
Copy link
Contributor Author

jangko commented Jun 5, 2018

#7955 and #7956 can be safely excluded, although they were discovered during #4799 investigation.
this one is ready for review

zah
zah reviewed Jun 5, 2018
View reviewed changes
tests/typerel/t4799_2.nim
@@ -0,0 +1,24 @@
discard """
Copy link
Member

@zah zah Jun 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps this bug doesn't deserve so many separate test cases (they slow down the execution of the test suite a little bit).

The alternative would be to use the accept/reject approach seen here:
https://github.com/nim-lang/Nim/blob/devel/tests/cpp/tcovariancerules.nim#L168

Araq
Araq reviewed Jun 6, 2018
View reviewed changes
tests/typerel/t4799.nim Outdated
var b = Bike[int](tire: 2)

reject:
echo testVehicle b, c, v
Copy link
Member

Choose a reason for hiding this comment

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

Wrong test, needs to be

reject:
  echo testVehicle(b, c, v)

Araq
Araq reviewed Jun 6, 2018
View reviewed changes
tests/typerel/t4799.nim Outdated
var b = Bike(tire: 2)

reject:
echo testVehicle b, c, v
Copy link
Member

Choose a reason for hiding this comment

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

same problem.

Araq
Araq reviewed Jun 6, 2018
View reviewed changes
tests/typerel/t4799.nim Outdated
var pgb = PGBike[int](tire: 2)

reject:
echo testVehicle pgb, pgc
Copy link
Member

Choose a reason for hiding this comment

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

same problem.

Araq
Araq reviewed Jun 6, 2018
View reviewed changes
tests/typerel/t4799.nim Outdated
var rb = RBike(tire: 2)

reject:
echo testVehicle rb, rc
Copy link
Member

Choose a reason for hiding this comment

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

same problem.

@Araq
Copy link
Member

Araq commented Jun 6, 2018

I'm accepting this but in the future please add more tests that should fail the type checker. A language where everything has a valid interpretation is a language that doesn't detect mistakes.

@Araq Araq merged commit bf394ed into nim-lang:devel Jun 6, 2018
@jangko jangko deleted the fix4799 branch July 20, 2018 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Araq Araq Araq left review comments

@zah zah zah left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jangko @Araq @zah