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

Create Symbol for unknown symbols (closes #876) #887

Merged
merged 27 commits into from
Sep 22, 2020
Merged

Conversation

arporter
Copy link
Member

@arporter arporter commented Sep 8, 2020

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Sep 8, 2020

Codecov Report

Merging #887 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #887   +/-   ##
=======================================
  Coverage   98.54%   98.54%           
=======================================
  Files         143      143           
  Lines       29367    29394   +27     
=======================================
+ Hits        28939    28966   +27     
  Misses        428      428           
Impacted Files Coverage Δ
src/psyclone/psyir/frontend/fparser2.py 99.04% <100.00%> (-0.01%) ⬇️
src/psyclone/psyir/nodes/node.py 95.74% <100.00%> (-0.03%) ⬇️
src/psyclone/psyir/symbols/datasymbol.py 100.00% <100.00%> (ø)
src/psyclone/psyir/symbols/symbol.py 100.00% <100.00%> (ø)
src/psyclone/psyir/symbols/symboltable.py 100.00% <100.00%> (ø)
...one/tests/psyir/frontend/fparser2_literals_test.py 100.00% <100.00%> (ø)
src/psyclone/tests/psyir/frontend/fparser2_test.py 100.00% <100.00%> (ø)
src/psyclone/transformations.py 99.22% <100.00%> (+<0.01%) ⬆️

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 7d6fffb...9cc6f5d. Read the comment docs.

@arporter arporter marked this pull request as draft September 9, 2020 15:07
@arporter arporter self-assigned this Sep 9, 2020
@arporter arporter added the PSyIR Core PSyIR functionality label Sep 9, 2020
@arporter
Copy link
Member Author

Missed coverage showed that we had duplicated the functionality of generating a Symbol for a kind parameter. I've now removed this duplication. We also had fparser2._get_symbol_table() which has been superseded by Rupert's new node.scope.symbol_table functionality so I've removed that too.

@arporter
Copy link
Member Author

Functionality wise, this is a small change to use Symbol rather than DataSymbol when we encounter a symbol of unknown type. However, this then required generalisation of some functionality (esp. the copy method) and I've removed some code duplication.
Ready for first review from either @sergisiso or @rupertford.

Copy link
Collaborator

@sergisiso sergisiso left a comment

Choose a reason for hiding this comment

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

@arporter I made a first review of the high-level implementation aspects, I made some suggestions about the interface, but in general I like the simplifications of code that this PR brings (although I need more context regarding the changes of the symbol copy method).

src/psyclone/psyir/frontend/fparser2.py Outdated Show resolved Hide resolved
src/psyclone/psyir/frontend/fparser2.py Outdated Show resolved Hide resolved
src/psyclone/psyir/frontend/fparser2.py Outdated Show resolved Hide resolved
src/psyclone/psyir/frontend/fparser2.py Outdated Show resolved Hide resolved
src/psyclone/psyir/symbols/datasymbol.py Show resolved Hide resolved
src/psyclone/psyir/symbols/datasymbol.py Outdated Show resolved Hide resolved
src/psyclone/psyir/symbols/symbol.py Outdated Show resolved Hide resolved
src/psyclone/psyir/symbols/symbol.py Show resolved Hide resolved
src/psyclone/transformations.py Outdated Show resolved Hide resolved
@arporter
Copy link
Member Author

Assuming Travis is happy, this is ready for next review.

@sergisiso
Copy link
Collaborator

@arporter I see that the issue mentioned in the title (issue 876) is still not solved with this PR, and in fact there are 2 TODOs still mentioning this issue int he code. So this is just some work needed to do towards solving the issue. Is that right?

@arporter
Copy link
Member Author

No, this should close #876. Let me take a look at the TODOs I've missed...

@arporter
Copy link
Member Author

Well, that was really weird - I appear to have simply not altered the routine that was supposed to be at the heart of this PR! I thought maybe I could blame a merge with master but I've not found any evidence for that. In fact 'fixing' find_or_create_symbol() actually required me to update one of the tests too so it seems unlikely that I ever did it.
Many thanks for spotting this!
Once Travis has reported this should be ready for review.

Copy link
Collaborator

@sergisiso sergisiso left a comment

Choose a reason for hiding this comment

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

@arporter I removed myself one last unused symbol. This PR is now ready to be merged.

@sergisiso sergisiso merged commit 055cca2 into master Sep 22, 2020
@sergisiso sergisiso deleted the 876_unknown_symbols branch September 22, 2020 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PSyIR Core PSyIR functionality ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants