Skip to content

(Closes #2812) max and min code update#3408

Open
LonelyCat124 wants to merge 10 commits intomasterfrom
2812_max_and_min_code_fix
Open

(Closes #2812) max and min code update#3408
LonelyCat124 wants to merge 10 commits intomasterfrom
2812_max_and_min_code_fix

Conversation

@LonelyCat124
Copy link
Copy Markdown
Collaborator

Small PR to ensure the max2code and min2code transformations use the datatype of their arguments correctly, and don't assume scalar real.

Ready for review from @arporter when he's back - its possible I missed something I should have added but I'm not sure.

@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.96%. Comparing base (ddc5397) to head (85cfcc2).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3408   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files         389      389           
  Lines       54541    54548    +7     
=======================================
+ Hits        54522    54529    +7     
  Misses         19       19           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@arporter arporter left a comment

Choose a reason for hiding this comment

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

Nice, thanks Aidan. Just a bit of docstring tidying to do and there's a (simple) test failure that needs fixing. Once you've done that, please could you launch the ITs too...

Check that it is safe to apply the transformation to the supplied node.

:param node: the SIGN call to transform.
:type node: :py:class:`psyclone.psyir.nodes.IntrinsicCall`
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

typehints please.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Added, also added the kwargs that I'd missed and fixed the typehints. I removed the options typehints since they're deprecated now.

@LonelyCat124
Copy link
Copy Markdown
Collaborator Author

Am hoping the test will just work this time as I'm a bit confused as I can't replicate it locally (any more). If not I'll dive deeper.

@LonelyCat124
Copy link
Copy Markdown
Collaborator Author

@arporter I have "fixed" the tree sitter issues now I think. I would ask if you could test locally as well and check you have no issues with your local version. I'm not sure why these tests have suddenly started failing, I tried running on another branch as well and they also have failures that weren't present last week. As far as I can find there has been no treesitter or treesitter-fortarn new release so its very strange.

@arporter
Copy link
Copy Markdown
Member

Hi @LonelyCat124, I've just tried locally (with 3.13) and I get one treesitter-related failure with this branch. If I switch to latest master it goes away. It could be something to do with tests leaking state?

@LonelyCat124
Copy link
Copy Markdown
Collaborator Author

Set the ITs going - assume they are successful this is ready for another look.

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.

2 participants