-
-
Notifications
You must be signed in to change notification settings - Fork 606
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
Implement pragma(ctfe) [alternative to #11007] #11014
Conversation
Thanks for your pull request and interest in making D better, @jpf91! 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 referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + dmd#11014" |
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.
Nice! It does look better and self-contained. I agree that D already has enough magic cases.
test/compilable/test_ctfeonly.d
Outdated
// REQUIRED_ARGS: -betterC | ||
// POST_SCRIPT: compilable/extra-files/ctfeonly-postscript.sh | ||
// I guess the platforms below don't have nm | ||
// DISABLED: win32 win64 osx |
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.
OSX does, and for windows you'll need dumpbin: https://docs.microsoft.com/en-us/cpp/build/reference/dumpbin-reference?view=vs-2019
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.
OK, let's see whether I can get this to work on windows. Do you know how to detect the OS in postscripts? But let's see, maybe uname works.
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.
You can use the OS
variable, see test/README.md
Do you think there's any way we could support declaring a function as CTFE from another library? For example, say I have a betterC program, but I want to call a function |
@marler8997 The only real solution to this problem that I see what I proposed in the forums: |
@marler8997 @PetarKirov The first thing to note is that DMD does already not complain if you use external functions in a CTFE-only context: import phobos;
void main()
{
mixin(generateMixin("int a = 42"));
} Where phobos has been compiled without Adding a pragma which affects other symbols is something quite new and probably controversial. And it would be strange, as it only affects templates, not functions. However, the solution here is actually quite simple: If a template is only instantiated from CTFE contexts, do not emit its symbols. This is similar to what speculative instantiation already does. E.g. this: import test2;
void main()
{
static if(__traits(compiles, generateMixin("int a = 42")))
pragma(msg, "it does");
} It compiles (somehow avoids -betterC checks, I'm not sure if that's a bug) and does not emit anything. We want exactly the same thing if a template is instantiated from a CTFE context. In the worst case, we might have to do semantic twice (first time: CTFE context, -betterC disabled, second time if function is used in other place in a runtime context, this time -betterC enabled). But apart from that detail, it should work just like speculative instantiation. This has the added benefit, that all templates benefit, without having to manually mark them as |
Maybe |
EDIT: see below comments, i retract this basically as inaccurate. Instead of as an alternative to the other one, I suggest both. Call the other PR "ctfe inference" if you will but do check the body. Why? A new pragma on the declaration is not compatible across dmd versions. Once you add it to code, you lock your code out of all previous versions of the compiler. And since it is attached to a declaration, the only way to version it out is to copy/paste the whole declarations which is obnoxious, and even if you try to factor the body out to another function, you can't mark that other function with the pragma so it defeats the purpose. And if any one of your libs try to use the new thing, it is liable to force a version-lock on its users too.... |
That's not entirely true, dmd has But adding this flag might be cumbersome depending on your current build system (if any). |
huh you know i either never knew that or forgot about it. and gdc has -fignore-unknown-pragmas Ignore unsupported pragmas. and ldc calls it --ignore OK, I take that back, well, 3/4 of the way back, it is still a slight annoyance but can document that for users. so i think i can live with it. |
As I imagine pragma to be used more often in the future, e.g. Anyhow, that's a bit OT to this PR as there are a lot of other good application of per-module compiler flags (like safe-by-default or other preview flags). |
generated object files. Although the linker usually removes those unused functions, | ||
this still causes some problems: | ||
|
||
* The function must pass semantic like any other function (i.e. with `-betterC`, |
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.
Please take care of trailing whitespace in here.
@jpf91
@jpf91 - This requires your attention. Please address all concerns and update to resolve all test failures. |
Rebooted here: #14036 |
A proof-of-concept alternative to #11007 , based on that PR.
Instead of detecting
assert(__ctfe)
, this one introducespragma(__ctfe)
. Compared to #11007 , in this one the information that a function is used only in CTFE is available earlier in the semantic phase. This allows the check to be integrated with-betterC
checks and allows all test to work in betterC mode:Whether this PR or the other is merged in the end, I strongly suggest that we make sure least the test cases above work fine, to integrate this properly with betterC.