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

Query: Translate Math.PI #25049

Closed
bricelam opened this issue Jun 7, 2021 · 5 comments
Closed

Query: Translate Math.PI #25049

bricelam opened this issue Jun 7, 2021 · 5 comments
Labels
closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed.

Comments

@bricelam
Copy link
Contributor

bricelam commented Jun 7, 2021

See #24829 (comment)

@bricelam bricelam added type-enhancement good first issue This issue should be relatively straightforward to fix. area-query labels Jun 7, 2021
@ajcvickers ajcvickers added this to the Backlog milestone Jun 8, 2021
@Marusyk
Copy link
Member

Marusyk commented Jun 17, 2021

I will try to prepare a PR for this

@smitpatel smitpatel removed this from the Backlog milestone Jul 9, 2021
@smitpatel smitpatel added closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. and removed good first issue This issue should be relatively straightforward to fix. labels Jul 9, 2021
@smitpatel
Copy link
Contributor

In the associated PR, we came to know that Math.PI is inlined as a constant value inside expression by C# compiler directly. This makes us never see Math.PI member expression in tree itself so this is not something we can interpret and translate. The constant value will always be sent to server as constant, as we receive it from C# compiler.

@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
@bricelam
Copy link
Contributor Author

Crazy idea: Replace Expression.Constant(Math.PI) with Expression.MakeMemberAccess(null, typeof(Math).GetField("PI")) in the preprocessor. Obviously, this would catch any constant that happens to equal .NET's approximation of PI, but is that really an issue?

If we do this, we should also do it for E and Tau.

@bricelam bricelam reopened this Jul 25, 2023
@ajcvickers
Copy link
Contributor

Note from triage: funcletization complicates this, and there seems to be very little value. Closing.

@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Jul 27, 2023
@roji
Copy link
Member

roji commented Jul 27, 2023

Technically the complication is even before ParameterExtractingEV (funcletization) - as above since Math.PI is a constant, the incoming expression tree doesn't even contain it, only the inlined constant value. So I think there's really nothing we can do here, even in theory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed.
Projects
None yet
Development

No branches or pull requests

5 participants