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

Method implementations 1: Variable references #253

Merged
merged 21 commits into from
Aug 30, 2021
Merged

Method implementations 1: Variable references #253

merged 21 commits into from
Aug 30, 2021

Conversation

ulmer-a
Copy link
Collaborator

@ulmer-a ulmer-a commented Aug 18, 2021

  • Let method implementations reference member variables of the containing class

@codecov
Copy link

codecov bot commented Aug 18, 2021 •
edited
Loading

Codecov Report

Merging #253 (6880162) into master (67dafe9) will increase coverage by 0.05%.
The diff coverage is 96.50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #253      +/-   ##
==========================================
+ Coverage   94.83%   94.89%   +0.05%     
==========================================
  Files          41       42       +1     
  Lines       10926    11137     +211     
==========================================
+ Hits        10362    10568     +206     
- Misses        564      569       +5
Impacted Files Coverage Δ
tests/tests.rs 100.00% <ø> (ø)
src/codegen/generators/expression_generator.rs 87.37% <87.50%> (-0.15%) ⬇️
src/codegen/generators/data_type_generator.rs 94.87% <94.25%> (+0.31%) ⬆️
src/resolver.rs 96.17% <98.07%> (+0.29%) ⬆️
src/ast.rs 89.91% <100.00%> (+0.17%) ⬆️
src/codegen.rs 100.00% <100.00%> (ø)
src/codegen/generators/pou_generator.rs 96.18% <100.00%> (+0.57%) ⬆️
src/codegen/generators/statement_generator.rs 99.80% <100.00%> (+<0.01%) ⬆️
src/codegen/generators/struct_generator.rs 98.88% <100.00%> (+0.03%) ⬆️
src/codegen/generators/variable_generator.rs 100.00% <100.00%> (ø)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67dafe9...6880162. Read the comment docs.

ulmer-a
ulmer-a commented Aug 18, 2021
View reviewed changes
Copy link
Collaborator Author

@ulmer-a ulmer-a left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure on how to really get information on my parent POU. Neither indexing nor annotating is performed hierarchically, so only the name of our POU tells us whether it has a parent an where it can be located. Is this a legitimate approach?

Ok I'm adding an Option in Pou which denotes the associated class for a given Method.

ulmer-a
ulmer-a commented Aug 19, 2021
View reviewed changes
Copy link
Collaborator Author

@ulmer-a ulmer-a left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@riederm Very nice and readable code in resolver.rs. Pleasure to work with.

The changes in the data type generator have nothing to do with the actual changes of this PR. It's just some refactoring to make clippy happy (there was some function with ~10 arguments).

I'll add some more tests soon.

@ulmer-a ulmer-a changed the title Method implementations (WIP) Method implementations Aug 19, 2021
@ulmer-a ulmer-a changed the title Method implementations Method implementations 1: Variable references Aug 19, 2021
@ulmer-a ulmer-a marked this pull request as ready for review August 19, 2021 12:29
@ulmer-a ulmer-a added this to the Object Oriented ST milestone Aug 19, 2021
@ulmer-a ulmer-a requested review from ghaith and riederm August 23, 2021 18:08
ghaith
ghaith reviewed Aug 24, 2021
View reviewed changes
Copy link
Collaborator

@ghaith ghaith left a comment

Choose a reason for hiding this comment

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

The resolution/declaration is looking good.
We are missing some tests (and functionality) regarding method calls, I will upload them soon.
Basically the codegen does not pass the method parameters to the call.
I will also add some correctness tests.

@ghaith
Copy link
Collaborator

ghaith commented Aug 24, 2021 •
edited
Loading

I uploaded failing tests
It looks like we still have issues with method returns, and methods as references.
We also can't seem to call method parameters.

@ghaith
Copy link
Collaborator

ghaith commented Aug 27, 2021 •
edited
Loading

Since I did the last few modifications I would prefer @riederm to review this.

@ghaith ghaith linked an issue Aug 27, 2021 that may be closed by this pull request
Support Classes : Part 2 Code generation #228
Closed
@ghaith ghaith mentioned this pull request Aug 27, 2021
Closed
@riederm
store the associated class of an impl in pou_type
320353e 
dont store an optional associated_class in the AST
implementation, but store the classname inside the pouType
variant "Class"
riederm
riederm requested changes Aug 30, 2021
View reviewed changes
Copy link
Collaborator

@riederm riederm left a comment

Choose a reason for hiding this comment

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

ill upload the changes

@riederm riederm merged commit 6fe9bc1 into master Aug 30, 2021
@riederm riederm deleted the method_impl branch August 30, 2021 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ghaith ghaith ghaith left review comments

@riederm riederm riederm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
No open projects
Next
Status: Done
Milestone
Object Oriented ST
Development

Successfully merging this pull request may close these issues.

Support Classes : Part 2 Code generation
3 participants
@ulmer-a @ghaith @riederm