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

emit("here"): "c code" + other emit fixes; new module experimental/backendutils.nim #13953

Closed
wants to merge 9 commits into from

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Apr 11, 2020

fixes A3,A4,A5 from #13943, adds a sane emit syntax, and new experimental module experimental/backendutils.nim

P1

new emit syntax: emit("typeSection"): "some C code"

this replaces the very hacky emit: "/TYPESECTION*/some C code" which continues to work but is now discouraged and will eventually be deprecated (with a warning). As mentioned by @krux02 #13943 (comment)

Unrelated to that, I would really like to have an emit prgama that has an enum argument to control the location. You know the usual problems with these magic comments. No error or warning when you mistype them. No error if the version of the compiler you are using doesn't support that value. No self explanatory API.

The new API is safe and will give CT error if section is invalid.
(also avoids silent errors like #15244 (comment) where a missing space caused obscure error)

P2

new emit section: emit("here"): "some C code emitted here"

Since nim code can appear anywhere including at module scope, it makes sense to allow emitting "right here" instead of at some file section.

As an example use case, this enables writing a correct c_sizeof, ie, fixing Example 3 from #13943, so that it works regardless of where it's called from (top-level, block-level or proc-level)
See tests for more examples.

P3

emit at block scope now uses section=here instead of being treated as module scope.
This fixes example 2 from #13943

P4

new experimental module experimental/backendutils.nim
This experimental module enables introspecting the generated file (eg C,C++), for tests, debugging, and user code.

P5

see tests tests/stdlib/tbackendutils.nim which has tests for everything

P6

new syntax for pragmas:
{.foo(args): baz.} is now allowed, and rewritten as: {.foo(args, baz).} (where args represents 1 or more arguments).
This enables the

{.emit("typeSection"): """
some code
""".}` 

but is generally useful and matches the way the rest of nim syntax works

after PR

  • update docs regarding emit, add c_offsetof, c_alignof and refactor with tsizeof.nim

@timotheecour timotheecour changed the title emit(here): "c code" + other emit fixes; new module experimental/backendutils.nim emit("here"): "c code" + other emit fixes; new module experimental/backendutils.nim Apr 11, 2020
@timotheecour timotheecour mentioned this pull request Apr 11, 2020
6 tasks
@timotheecour timotheecour changed the title emit("here"): "c code" + other emit fixes; new module experimental/backendutils.nim [WIP] emit("here"): "c code" + other emit fixes; new module experimental/backendutils.nim Apr 11, 2020
@timotheecour timotheecour reopened this Apr 11, 2020
@timotheecour timotheecour mentioned this pull request Apr 11, 2020
1 task
@timotheecour timotheecour changed the title [WIP] emit("here"): "c code" + other emit fixes; new module experimental/backendutils.nim emit("here"): "c code" + other emit fixes; new module experimental/backendutils.nim Apr 11, 2020
@krux02
Copy link
Contributor

krux02 commented Apr 11, 2020

I have an idea and would like to hear your opinion about it:

I personally don't like the pragma notation for emit. I also don't like multi line string literals. So this is my idea to avoid it:

template c_sizeof*(T: typedesc): int =
  var s: int
  # the symbols s and T are captured
  asm(efHere, [s, T]):
    s = sizeof(T); // remember, this is C code without quotes in Nim code
  s

This is how it can work: I use the asm keyword here instead of emit. Asm is a keyword in the language, therefore the Nim parser already knows about it. The parser knows that this asm statement is followed by a block of indented C code. The parser can then eat the entire block of injected C code and process it like a multiline string literal. The C code ends like all other blocks of Nim end as well, with their indentation. The injected symbols are listed at the top as an explicit list, as it was planned in my quoteAst branch.

The reason for asm over emit is only because asm is a keyword. No other reason. It would also be wrong to call it asm because it isn't assembly. But asm something that I would see as technically possible without breaking anything.

@timotheecour
Copy link
Member Author

timotheecour commented Apr 11, 2020

I have an idea and would like to hear your opinion about it:

I've opened nim-lang/RFCs#210 to followup on your proposal, 1 part of your proposal is good and should be implemented, see my comments there; that being said it's orthogonal to this PR (in the sense that either/both can be implemented).

@Araq
Copy link
Member

Araq commented Apr 14, 2020

Why do we need even more emit hacks? Makes me want to use NLVM already.

@timotheecour
Copy link
Member Author

timotheecour commented Apr 14, 2020

Why do we need even more emit hacks? Makes me want to use NLVM already.

On the contrary, I view the existing semantics as a hack: emit: "/TYPESECTION*/some C code" where if you mistyped it, you'll get no warnings/errors but silently maybe-wrong results:

as @krux02 said here #13943 (comment)

I would really like to have an emit prgama that has an enum argument to control the location. You know the usual problems with these magic comments. No error or warning when you mistype them. No error if the version of the compiler you are using doesn't support that value. No self explanatory API.

Furthermore, this PR enables implementing things like a correct c_sizeof (etc) that works everywhere (not just at proc scope), see experimental/backendutils.nim + tests/stdlib/tbackendutils.nim for examples.

@Varriount
Copy link
Contributor

While I understand it's necessity, using emit intrinsically require assumptions about how backend code is generated. Declaring that "this is an officially supported way of getting backend detail X" means that backend code generation would be subject to (more, if any currently exist) backwards compatibility restrictions.

@krux02
Copy link
Contributor

krux02 commented Apr 15, 2020

Why do we need even more emit hacks? Makes me want to use NLVM already.

That would be a breaking change. It would not only break all wrappers that wrap C macros it would also make it impossible to fix them. Of course I am not telling you something new. I just want to know why you want so desperately to break this.

@Araq
Copy link
Member

Araq commented Apr 15, 2020

Well no, rejected. Since emit is a hack there is no point in fixing it or putting even more logic into the compiler so that you can mis-use it more frequently and in a "safer" way, it makes no sense.

@Araq Araq closed this Apr 15, 2020
@nim-lang nim-lang deleted a comment from krux02 Apr 15, 2020
@timotheecour
Copy link
Member Author

there is no point in fixing

that makes no sense for code that needs to use it, eg code that is interop heavy. emit is already labeled as a sharp feature in the docs, no need to cripple it when there is no good workaround.

@Araq
Copy link
Member

Araq commented Apr 15, 2020

It's not "crippled", it's simply not as typo-safe as you would like.

@timotheecour
Copy link
Member Author

timotheecour commented Apr 15, 2020

But I have real use cases for emit("here"), introduced in this PR. What's the workaround for that?
I'm fine improving a PR to address technical insufficiencies, but closing it altogether doesn't solve the underlying problems.

@Araq
Copy link
Member

Araq commented Apr 15, 2020

What's the real use case? This needs to be justfiied way better and also should be backed up by an accepted RFC.

@Araq Araq reopened this Apr 15, 2020
@timotheecour
Copy link
Member Author

thank you for re-opening. I will will follow-up with an RFC, but that will take some time.

@stale
Copy link

stale bot commented Sep 18, 2021

This pull request has been automatically marked as stale because it has not had recent activity. If you think it is still a valid PR, please rebase it on the latest devel; otherwise it will be closed. Thank you for your contributions.

@stale stale bot added the stale Staled PR/issues; remove the label after fixing them label Sep 18, 2021
@Araq
Copy link
Member

Araq commented Sep 19, 2021

No RFC has been written. We can always re-open it later.

@Araq Araq closed this Sep 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants