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

Remove unnecessary dtype property from Declare #1730

Merged
merged 18 commits into from
Feb 15, 2024
Merged

Remove unnecessary dtype property from Declare #1730

merged 18 commits into from
Feb 15, 2024

Conversation

EmilyBourne
Copy link
Member

@EmilyBourne EmilyBourne commented Feb 14, 2024 •
edited
Loading

Currently the class Declare takes a dtype. In the C translation this value is ignored. In the Fortran translation it is used but other typing details are collected from the variable. Whenever the Declare instance is created the dtype parameter passed is always collected from the Variable. This is quite repetitive and unnecessary. This PR removes this redundancy and duplication. This will remove an unnecessary complexity when modifying datatype handling (see #1722).

Commit Summary

  • Remove unnecessary ast.core.Declare.dtype parameter
  • Remove unnecessary ast.core.Declare.passed_from_dotted parameter
  • Remove the unused class ast.core.Block
  • Simplify storage of declarations in Fortran printer to avoid using dictionary (without using keys)
  • Remove dead code

@pyccel-bot
Copy link

pyccel-bot bot commented Feb 14, 2024

Hello again! Thank you for this new pull request 🤩.

Please begin by requesting your checklist using the command /bot checklist

@EmilyBourne
Copy link
Member Author

EmilyBourne commented Feb 14, 2024 •
edited
Loading

Here is your checklist. Please tick items off when you have completed them or determined that they are not necessary for this pull request:

  • Write a clear PR description
  • Add tests to check your code works as expected
  • Update documentation if necessary
  • Update Changelog
  • Ensure any relevant issues are linked
  • Ensure new tests are passing

@EmilyBourne
Copy link
Member Author

/bot run docs coverage

EmilyBourne
EmilyBourne commented Feb 14, 2024
View reviewed changes
pyccel/ast/core.py Outdated Show resolved Hide resolved
@EmilyBourne
Copy link
Member Author

/bot run docs coverage

@EmilyBourne
Copy link
Member Author

/bot run docs

@EmilyBourne
Copy link
Member Author

/bot run docs coverage

pyccel-bot[bot]
pyccel-bot bot suggested changes Feb 14, 2024
View reviewed changes
Copy link

@pyccel-bot pyccel-bot bot left a comment

Choose a reason for hiding this comment

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

There seems to be lines in this PR which aren't tested. Please take a look at my comments and add tests which cover the new code.

If this is modified code which cannot be easily tested in this PR please open an issue to request that this code be either removed or tested. Once you have done that please leave a message on the relevant conversation beginning with the line /bot accept and referencing the issue.

Similarly if the new code cannot be tested for some reason, please leave a comment beginning with the line /bot accept on the relevant conversation explaining why the code can't be tested.

pyccel/codegen/printing/fcode.py Outdated Show resolved Hide resolved
pyccel/codegen/printing/fcode.py Outdated Show resolved Hide resolved
@EmilyBourne
Copy link
Member Author

/bot run docs coverage

@EmilyBourne
Copy link
Member Author

/bot run docs coverage

@EmilyBourne EmilyBourne added the internals Pyccel's internal behavior, does not affect user experience label Feb 15, 2024
pyccel-bot[bot]
pyccel-bot bot approved these changes Feb 15, 2024
View reviewed changes
Copy link

@pyccel-bot pyccel-bot bot left a comment

Choose a reason for hiding this comment

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

Good job ! Your PR is using all the code it added/changed.

@EmilyBourne EmilyBourne marked this pull request as ready for review February 15, 2024 08:26
@pyccel-bot
Copy link

pyccel-bot bot commented Feb 15, 2024

Hey @pyccel/pyccel-dev ! @EmilyBourne has just created this great new pull request! Check it out and let me know what you think!

@pyccel-bot pyccel-bot bot requested a review from a team February 15, 2024 08:54
Farouk-Echaref
Farouk-Echaref approved these changes Feb 15, 2024
View reviewed changes
Copy link
Contributor

@Farouk-Echaref Farouk-Echaref left a comment

Choose a reason for hiding this comment

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

Looks good to me

@pyccel-bot pyccel-bot bot added Ready_for_review Received at least one approval. Requires review from senior developer and removed needs_initial_review labels Feb 15, 2024
@pyccel-bot
Copy link

pyccel-bot bot commented Feb 15, 2024

Hey @yguclu, @EmilyBourne, this PR is looking pretty good. @EmilyBourne and @Farouk-Echaref think it is ready to merge. Could you add your expertise to confirm that this follows all the coding conventions and fits in Pyccel's future plans? Thanks 😄

yguclu
yguclu reviewed Feb 15, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@EmilyBourne @yguclu
Specify modules
e955119 
Co-authored-by: Yaman Güçlü <yaman.guclu@gmail.com>
yguclu
yguclu approved these changes Feb 15, 2024
View reviewed changes
Copy link
Member

@yguclu yguclu left a comment

Choose a reason for hiding this comment

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

Looks good to me. I am always glad to see some code clean up!

@pyccel-bot pyccel-bot bot added Ready_to_merge Approved by senior developer. Ready for final approval and merge and removed Ready_for_review Received at least one approval. Requires review from senior developer labels Feb 15, 2024
@yguclu
Copy link
Member

yguclu commented Feb 15, 2024

/bot run pr_tests

pyccel-bot[bot]
pyccel-bot bot approved these changes Feb 15, 2024
View reviewed changes
Copy link

@pyccel-bot pyccel-bot bot left a comment

Choose a reason for hiding this comment

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

Good job ! Your PR is using all the code it added/changed.

@pyccel-bot pyccel-bot bot added Ready_to_merge Approved by senior developer. Ready for final approval and merge and removed Ready_to_merge Approved by senior developer. Ready for final approval and merge labels Feb 15, 2024
@yguclu yguclu merged commit 4ac86f2 into devel Feb 15, 2024
12 checks passed
@yguclu yguclu deleted the ebourne_declare_dtype branch February 15, 2024 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yguclu yguclu yguclu approved these changes

@Farouk-Echaref Farouk-Echaref Farouk-Echaref approved these changes

@pyccel-bot pyccel-bot[bot] pyccel-bot approved these changes

Assignees
No one assigned
Labels
internals Pyccel's internal behavior, does not affect user experience Ready_to_merge Approved by senior developer. Ready for final approval and merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@EmilyBourne @yguclu @Farouk-Echaref