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

Implement __ctfeWrite builtin #16250

Merged
merged 5 commits into from Mar 18, 2024
Merged

Conversation

JohanEngelen
Copy link
Contributor

Reworked stalled PR #12412.
The __ctfeWrite function is already part of druntime (core.builtins), but was never implemented. The actual implementation is the one used at Weka (slightly different from PR #12412) and does not require any changes to expression.d (contrary to said PR).

One open issue: currently stderr is hardcoded. Is this "implementation defined" or not, or should we add a second parameter that enables the user to select between stdout and stderr? I actually prefer this last option, i.e. changing signature to __ctfeWrite(scope const(char)[] s, bool to_stderr = true).
For reference, the output of pragma(msg, ...) to stderr is implementation defined (https://dlang.org/spec/pragma.html#msg)

Reworked stalled PR dlang#12412.
The __ctfeWrite function is already part of druntime (core.builtins), but was never implemented.
The actual implementation is the one used at Weka (slightly different from PR dlang#12412) and does not require any changes to expression.d (contrary to said PR).
@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @JohanEngelen! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

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

Your 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 locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#16250"

@@ -448,6 +448,8 @@ immutable Msgtable[] msgtable =
{ "outp"},
{ "outpl"},
{ "outpw"},
{ "builtinsModuleName", "builtins" },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note the conflict with importC's builtins.d druntime file; that one has a very unfortunate (and confusing) name...

@JohanEngelen JohanEngelen added the Industry Applies to PRs pertaining to industry applications of D label Feb 25, 2024
@RazvanN7
Copy link
Contributor

currently stderr is hardcoded. Is this "implementation defined" or not, or should we add a second parameter that enables the user to select between stdout and stderr? I actually prefer this last option, i.e. changing signature to __ctfeWrite(scope const(char)[] s, bool to_stderr = true)

Weka has more experience with this, so I suggest doing what fits you best.

Also, we should probably also have some documentation for this feature even if it's not intended for the regular user to use it.

@JohanEngelen
Copy link
Contributor Author

One open issue: currently stderr is hardcoded. Is this "implementation defined" or not, or should we add a second parameter that enables the user to select between stdout and stderr? I actually prefer this last option, i.e. changing signature to __ctfeWrite(scope const(char)[] s, bool to_stderr = true). For reference, the output of pragma(msg, ...) to stderr is implementation defined (https://dlang.org/spec/pragma.html#msg)

@CyberShadow @ibuclaw

@ibuclaw
Copy link
Member

ibuclaw commented Feb 26, 2024

If written to stdout, don't be surprised if you get assembler errors when combined with -pipe

@JohanEngelen
Copy link
Contributor Author

If written to stdout, don't be surprised if you get assembler errors when combined with -pipe

Do you mean that it would be hard/impossible to implement "to stdout" for GDC ? So that it has to be implementation defined, anyways?

@ibuclaw
Copy link
Member

ibuclaw commented Feb 26, 2024

If written to stdout, don't be surprised if you get assembler errors when combined with -pipe

Do you mean that it would be hard/impossible to implement "to stdout" for GDC ? So that it has to be implementation defined, anyways?

Yes, stdout is reserved for the output file in GDC.

changelog/dmd.ctfeWrite.dd Outdated Show resolved Hide resolved
@JohanEngelen JohanEngelen marked this pull request as draft February 27, 2024 22:31
@JohanEngelen JohanEngelen marked this pull request as ready for review February 29, 2024 18:38
@JohanEngelen
Copy link
Contributor Author

PR is ready.
The earlier CI errors are the known macOS issues, unrelated to this PR.

@RazvanN7
Copy link
Contributor

RazvanN7 commented Mar 6, 2024

@JohanEngelen This also requires a spec PR.

@JohanEngelen
Copy link
Contributor Author

@JohanEngelen This also requires a spec PR.

Where?
This is not a language feature, but a druntime feature, no? Like any other builtin function. No spec change needed.

@RazvanN7
Copy link
Contributor

Well, it depends on how you look at it. The druntime implementation does nothing. As far as I could tell from the previous PRs the only point of having it declared in druntime is to be able to type check calls to __ctfeWrite properly (please correct me if I'm wrong). Therefore, the fact that __ctfeWrite is declared in druntime is just an implementation detail. I, personally, see ctfeWrite as a compiler facility to write strings during CTFE. As such, in my opinion, it should be documented accordingly. Right now, the only documentation we have on it is "Writes s to stderr during CTFE (does nothing at runtime" - which is confusing: why have a function that does nothing declared in druntime?. The changelog that you have written is much more clear and deserves to be put in the documentation so that users of the language know how to use this feature.

Arguably, having __ctfeWrite defined in druntime is something that could be avoided.

@JohanEngelen
Copy link
Contributor Author

The changelog that you have written is much more clear

The changelog entry is actually identical to the existing documentation of the druntime function! (literally almost word-for-word indentical).

Arguably, having __ctfeWrite defined in druntime is something that could be avoided.

Maybe. But none of the builtin functions are documented in the spec. So my question remains: Where do you want it in the spec?

@RazvanN7
Copy link
Contributor

Here: https://dlang.org/spec/function.html#interpretation . Next to __ctfe.

@RazvanN7
Copy link
Contributor

The changelog entry is actually identical to the existing documentation of the druntime function! (literally almost word-for-word indentical).

@JohanEngelen what do you mean? I don't see any documentation for the druntime function apart from: https://github.com/dlang/dmd/blob/master/druntime/src/core/builtins.d#L52 . Whereas the changelog is much more explanatory, it even provides an example (I would also add an example to highlight the difference between __ctfeWrite and pragma(msg)).

@WalterBright WalterBright merged commit 92181d3 into dlang:master Mar 18, 2024
46 of 48 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Industry Applies to PRs pertaining to industry applications of D Walter Bright
Projects
None yet
5 participants