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

Callref compilers #2793

Merged
merged 7 commits into from
Oct 25, 2019
Merged

Callref compilers #2793

merged 7 commits into from
Oct 25, 2019

Conversation

Louis-Vincent
Copy link

No description provided.

Copy link
Member

@privat privat left a comment

Choose a reason for hiding this comment

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

First batch of comments, I will finish latter

src/compiler/abstract_compiler.nit Outdated Show resolved Hide resolved
src/compiler/abstract_compiler.nit Outdated Show resolved Hide resolved
src/compiler/abstract_compiler.nit Outdated Show resolved Hide resolved
# Otherwise, we get a compile error exactly here. This weird behavior doesn't affect
# the inner mecanics of callref since the return type is already solved by
# `routine_ref_call`
if ret != null and not compiler.all_routine_types_name.has(cname) then
Copy link
Member

Choose a reason for hiding this comment

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

I couldn't reproduce the weird behavior. Do you have a minimal example? (also put it in the /tests directory)

Copy link
Author

@Louis-Vincent Louis-Vincent Oct 5, 2019

Choose a reason for hiding this comment

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

I added tests/test_callref.nit, this behavior only occured if you compile in semi-global.
Here the assertion result :

Runtime assert: RESULT.can_resolve_for(Fun0[Object], FunRef0[nullable Object], test_callref)

src/compiler/abstract_compiler.nit Outdated Show resolved Hide resolved
src/compiler/global_compiler.nit Outdated Show resolved Hide resolved
# -- and covariance. Thus, creating warnings when
# -- compiling in global. However, if you ignore
# -- those warnings, the binary works perfectly fine.
# ~~~~
Copy link
Member

Choose a reason for hiding this comment

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

I think we can live with the warning for now, --global it not a popular option. #2781 is problematic because its a new warning and cannot be disabled without breaking previous version of gcc. Maybe we will just disable all C warnings.

src/compiler/global_compiler.nit Outdated Show resolved Hide resolved
src/compiler/global_compiler.nit Show resolved Hide resolved
tests/syntax_callref.nit Outdated Show resolved Hide resolved
Copy link
Member

@privat privat left a comment

Choose a reason for hiding this comment

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

do not commit binary files: tests/syntax_callref

src/compiler/separate_compiler.nit Outdated Show resolved Hide resolved
src/compiler/separate_compiler.nit Outdated Show resolved Hide resolved
src/compiler/separate_compiler.nit Outdated Show resolved Hide resolved
src/rapid_type_analysis.nit Outdated Show resolved Hide resolved
src/rapid_type_analysis.nit Outdated Show resolved Hide resolved
@Louis-Vincent Louis-Vincent force-pushed the callref-compilers branch 2 times, most recently from af8eb24 to 556a50d Compare October 1, 2019 17:42
@Louis-Vincent Louis-Vincent requested a review from privat October 1, 2019 17:43
Copy link
Member

@privat privat left a comment

Choose a reason for hiding this comment

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

Space are both important and not important.

  • Use correct indentation on every piece of code you add or change: this improves the overall quality of the code
  • Do not change space or anything in pieces of code unrelated to the commit: otherwise, this make code review and blaming more complex

As a rule of thumb:

  • Beware of global code change (sed or text editor search&replace)
  • Check your commits do avoid unrelated change. Either interactively when doing them (git gui with "stage hunk/line" or git add -i) or before pushing (gitk or git log -p)

src/compiler/abstract_compiler.nit Outdated Show resolved Hide resolved
src/compiler/abstract_compiler.nit Outdated Show resolved Hide resolved
src/compiler/abstract_compiler.nit Outdated Show resolved Hide resolved
Comment on lines 2714 to 2718
# WARNING: we must not resolve the return type when it's a functional type.
# Otherwise, we get a compile error exactly here. This weird behavior doesn't affect
# the inner mecanics of callref since the return type is already solved by
# `routine_ref_call`
if ret != null and not compiler.all_routine_types_name.has(cname) then
Copy link
Member

Choose a reason for hiding this comment

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

This is really weird. Do you have a test-case? I cannot reproduce a case where the additional condition is needed.

src/compiler/global_compiler.nit Show resolved Hide resolved
src/compiler/global_compiler.nit Show resolved Hide resolved
Signed-off-by: Louis-Vincent Boudreault <[email protected]>
- Added test case in `src/compiler/test_callref.nit`
- Updated the API of `AbstractCompiler` to support routine reference
call and instantiation services.
- Implemented routine reference (callrefs) handling for separate
compiler

Signed-off-by: Louis-Vincent Boudreault <[email protected]>
Signed-off-by: Louis-Vincent Boudreault <[email protected]>
Signed-off-by: Louis-Vincent Boudreault <[email protected]>
@Louis-Vincent Louis-Vincent force-pushed the callref-compilers branch 2 times, most recently from d06716a to 21a2325 Compare October 10, 2019 14:47
Signed-off-by: Louis-Vincent Boudreault <[email protected]>
~~~~nitish
class A[E]
	fun foo: A[E] do return self
	fun bar: Fun0[A[E]] do return &foo # it didn't work before
end
~~~~

- Fixed a bug when `self` was a generic class with unsolved type. It was
impossible to return a callref, since the typing rule were not properly
executed.
- Added a test case in `test_callref.res`

Signed-off-by: Louis-Vincent Boudreault <[email protected]>
privat added a commit that referenced this pull request Oct 25, 2019
Pull-Request: #2793
Reviewed-by: Jean Privat <[email protected]>
@privat privat merged commit 31110c7 into nitlang:master Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants