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
[stable] Fix Issue 19219 - Could not CTFE with std.math.exp #8660
Conversation
Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla references
Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "stable + dmd#8660" |
@@ -124,6 +124,13 @@ extern (C++) Expression eval_log10(Loc loc, FuncDeclaration fd, Expressions* arg | |||
return new RealExp(loc, CTFloat.log10(arg0.toReal()), arg0.type); | |||
} | |||
|
|||
extern (C++) Expression eval_exp(Loc loc, FuncDeclaration fd, Expressions* arguments) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not my expertise but Walter has changed a lot of passing Loc
by value to pass by const ref
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, but the other builtins have the same signature, and add_builtin
requires it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, ok.
Win32: |
@@ -381,6 +388,7 @@ public extern (C++) void builtin_init() | |||
add_builtin("_D3std4math3tanFNaNbNiNeeZe", &eval_tan); | |||
add_builtin("_D3std4math4sqrtFNaNbNiNeeZe", &eval_sqrt); | |||
add_builtin("_D3std4math4fabsFNaNbNiNeeZe", &eval_fabs); | |||
add_builtin("_D3std4math3expFNaNbNiNeeZe", &eval_exp); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could that builtin have gone to druntime instead ? AFAIK that's what we are trying to do nowadays (well, since a few years), and this list should probably be purged since many of those builtin just forward to druntime AFAIK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mixed something up here. These CTFE builtins are generally used for non-CTFEable functions, e.g., if implemented in asm. None of the math ones use druntime IIRC, but the host's C functions.
The underlying issue is an apparent CTFE bug wrt. passing static immutable static arrays by (const) ref, see https://issues.dlang.org/show_bug.cgi?id=17351#c10. That makes this poly() optimization not CTFE-able.