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

Remove unreachable and unused overload methods from IntrinsicFunctions #8649

Closed
wants to merge 1 commit into from

Conversation

jrdodds
Copy link
Contributor

@jrdodds jrdodds commented Apr 8, 2023

Fixes # (issue to be created)

Context

This is a change that was split out from PR #8569 at the request of @rainersigwald.

The change removes type overload methods from IntrinsicFunctions that are unreachable via evaluation and that are unused within the repo.

This is cleanup that could support a future effort to add source generation for the 'fast path' lookup.

Changes Made

Remove long variants of Add, Subtract, Multiply, Divide, and Modulo. Evaluation will always use the double variants.

Testing

Built and ran unit tests on Windows 11 and macOS 12.

Notes

Copy link
Member

@JanKrivanek JanKrivanek 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 - dead code removal

@Forgind
Copy link
Member

Forgind commented Apr 24, 2023

rainersigwald commented this morning that our documentation says we support longs and made a pigeonhole argument that supporting doubles does not obviate the need to support long. Indeed, I made a very simple test project (see #8698) that demonstrates that MSBuild is indeed flawed in this respect. We're currently not 100% clear on whether we can take this change and just change how we parse the numbers or if we need to explicitly call these overloads but currently aren't.

@jrdodds
Copy link
Contributor Author

jrdodds commented Apr 24, 2023

This issue of the unsupported overloads came up in the discussion for #8569 but it is 'new' in this discussion that long must be supported. It does make sense that long should be supported and that parsing only to a double is a defect.

@jrdodds
Copy link
Contributor Author

jrdodds commented Apr 28, 2023

Negated by #8710

@jrdodds jrdodds closed this Apr 28, 2023
@jrdodds jrdodds deleted the IntrinsicFunctionsCleanUp branch April 28, 2023 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants