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

Cleanup initializer grammar #1441

Merged
merged 60 commits into from
Dec 20, 2022
Merged

Cleanup initializer grammar #1441

merged 60 commits into from
Dec 20, 2022

Conversation

oowekyala
Copy link
Collaborator

@oowekyala oowekyala commented Nov 1, 2022

Changes extracted from #544.

This does not change the LF language, it just introduces the node Initializer to fold some duplicated code in the grammar file. Later this can be changed to support the new = syntax described in #986

Since the code generation in the C++ target needed to be updated to adjust to the grammar changes, we also used the opportunity to improve the handling of parameters in C++ and fix a couple of related issues along the way.

Fixes #419
Fixes #657
Fixes #605

oowekyala and others added 8 commits November 23, 2022 18:35
The  and  syntax for parameter assignments
are now not handled at all - this causes errors with
generic types and is inconsistent with C++ syntax.
Todo add warning

Default parameters are now not generated anymore instead
of copying the expression to the call site
Some things do not work properly now:
- out-of-order default parameters
- implicit conversion of argument to parameter type

These should be fixed by generating the
parameters into a struct
Copy link
Collaborator

@cmnrd cmnrd left a comment

Choose a reason for hiding this comment

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

I think this PR is taking shape now and I will mark it as ready for review. We still have to fix a couple of tests though.

I fixed the C++ tests (for the most parts) and we now have passing tests for C, C++, and Python. There is still a couple of Windows tests failing which I will look into.

@oowekyala There appears to be a problem with multiports in Rust. Cold you take a look? This could be related to the fact that the rust generator uses toCppCode on the width.

@petervdonovan could I ask you to help with the failing unit tests (round trip) and the failing LSP tests? I don't understand why the unit test fails. It seems to insert some erroneous code into one of our test files but then expects the file to parse. For the LSP test, I was not able to find the actual error in the log. I am not sure what I need to search for her, but 'error' or 'fail' does not work. It would be great if we could further improve the outputs of theses tests so that the log clearly states where the error happened and also points to how it could be fixed.

@lhstrh The TS tests fail and likely the TS generators still need to be updated. Who could help with this?

@cmnrd cmnrd marked this pull request as ready for review December 13, 2022 17:11
@lhstrh
Copy link
Member

lhstrh commented Dec 13, 2022

If they have a chance, perhaps @hokeun or @byeong-gil could have a look at the TypeScript tests?

@cmnrd
Copy link
Collaborator

cmnrd commented Dec 14, 2022

Thanks for your fix @petervdonovan! So the round trip tests apply the formatting to our tests and check if the result is still valid LF?

@hokeun
Copy link
Member

hokeun commented Dec 14, 2022

If they have a chance, perhaps @hokeun or @byeong-gil could have a look at the TypeScript tests?

Sorry @lhstrh, I'm tied up with some other things right now, but I should be able to take a look at it this weekend!

@cmnrd
Copy link
Collaborator

cmnrd commented Dec 14, 2022

Actually I just fixed the TS tests. It turned out to be a simple fix. Sorry for the noise!

Copy link
Collaborator

@cmnrd cmnrd left a comment

Choose a reason for hiding this comment

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

Looks like all the tests pass now. As far as I am concerned, this is ready to be merged.

Copy link
Collaborator Author

@oowekyala oowekyala left a comment

Choose a reason for hiding this comment

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

Thanks for pushing this forward @cmnrd!

org.lflang/src/org/lflang/ASTUtils.java Outdated Show resolved Hide resolved
org.lflang/src/org/lflang/generator/c/CTypes.java Outdated Show resolved Hide resolved
Copy link
Collaborator

@petervdonovan petervdonovan left a comment

Choose a reason for hiding this comment

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

Thanks! This substantially cleans up and adds tests for some code that I left and the cleanups in the grammar look good to me. I was not able to find mistakes

@cmnrd cmnrd merged commit 1069cc4 into master Dec 20, 2022
@cmnrd cmnrd deleted the initializers-again branch December 20, 2022 09:40
@cmnrd cmnrd added the cleanup label Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants